Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make wrap fields also visible in entry editor #6315

Merged
merged 6 commits into from
May 8, 2020
Merged

Make wrap fields also visible in entry editor #6315

merged 6 commits into from
May 8, 2020

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Apr 19, 2020

Fixes #4373

I simple reused the FieldFormatterPreferences wrapFieldList. More like a hack.
The original multiline property got removed in #5230
A more elegant solution would be to maybe add this to the custom entry types dialog?

  • Change in CHANGELOG.md described (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's indeed a bit hacky. I like your idea to make this configurable since this is only one instance where people add a new field and require a certain type of action (e.g. what if I add a new "persons" field, or a new "keywords" field?). Thus having an option to specify the type of a field would be good.

* upstream/master:
  Update label names
  Squashed 'src/main/resources/csl-locales/' changes from d0ee4d13c9..79845b087b
  Squashed 'src/main/resources/csl-styles/' changes from c1793d2..143464e
  Cite work by @ayaankazerouni
  Improve performance for loading files (#6332)
  Add ADR von JUnit vs. AssertJ (#6335)
  'Name' textfield on focus instead of 'OK' button when user clicks on 'Add subgroup' option (#6330)
  Bump jurt from 6.3.2 to 6.4.3 (#6325)
  Bump unoil from 6.3.2 to 6.4.3 (#6320)
  Bump ridl from 6.3.2 to 6.4.3 (#6326)
  Bump classgraph from 4.8.69 to 4.8.71 (#6322)
  Bump flexmark from 0.61.6 to 0.61.16 (#6318)
  Bump richtextfx from 0.10.4 to 0.10.5 (#6319)
  Bump guava from 28.2-jre to 29.0-jre (#6323)
  Bump flexmark-ext-gfm-tasklist from 0.61.6 to 0.61.16 (#6327)
  Bump org.beryx.jlink from 2.17.5 to 2.17.7 (#6324)
  Improve performance massively by fixing a stupid binding mistake (#6316)
  Fixed missing paste command (#6313)
  Remove cache of auto completion results (#6310)
@Siedlerchr
Copy link
Member Author

I would leave a proper implementation for the future. This depends also a bit on #6152

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this fixes an issue, we really need a CHANGELOG.md entry.

@@ -52,10 +51,10 @@ public static FieldEditorFX getForField(final Field field,
preferences.getBoolean(JabRefPreferences.ENFORCE_LEGAL_BIBTEX_KEY),
preferences.getBoolean(JabRefPreferences.ALLOW_INTEGER_EDITION_BIBTEX));

final boolean isSingleLine = FieldFactory.isSingleLineField(field);
boolean isMultiLine = FieldFactory.isMultiLineField(field, preferences.getFieldContentParserPreferences().getNonWrappableFields());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use an enum here? I really don't like the booleans - and we see changing the flag from singleLine to multiLine was necessary of readability. (Otherweise, the variable would be kept to flag singleLine - and negation would be used)

I propose to use a boolean enum as proposed at https://stackoverflow.com/a/12713550/873282

enum USED_LINES {
    MULTI(true), SINGLE(false);
...

Maybe, just a simple enum would also be OK. Then, the construction from boolean would be a bit more tricky though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to go in this direction, an enum FieldType with Text and TextLong would make the most sense and conveys the semantics. Can then be extended in the future to include Keywords, Numeric etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really would like to leave it for the moment and implement a more generic solution in a follow up PR. I think about integrating it in the custom entry types dialog.

textInput = isSingleLine
? new EditorTextField()
: new EditorTextArea();
textInput = isMultiLine ? new EditorTextArea() : new EditorTextField();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that the true/false branches were just be wrapped. Another indicator for an enum!

* upstream/master:
  Backward compatibility for 4.3.1 (#6296)
  Fix Export to clipboard Dialog icon (#6345)
  Refactor EntryEditorPreferences (#6245)
@calixtus
Copy link
Member

calixtus commented May 8, 2020

Ping.
How do we proceed here?

@Siedlerchr
Copy link
Member Author

I m merge this now and will work on a more clean solution

@Siedlerchr Siedlerchr merged commit e0baa6d into master May 8, 2020
@Siedlerchr Siedlerchr deleted the multiline branch May 8, 2020 18:07
Siedlerchr added a commit that referenced this pull request May 8, 2020
…ptyLib

* upstream/master: (23 commits)
  Make wrap fields also visible in entry editor (#6315)
  Add launcher to fix extension import in snap (#6439)
  add concise message when SaveException happen (#6444)
  Squashed 'src/main/resources/csl-locales/' changes from 79845b087b..cbb45961b8
  Squashed 'src/main/resources/csl-styles/' changes from 906cd6d..270cd32
  Addition to the early pull #6438 (#6441)
  Fix the bug #6421 (#6438)
  Cleanup dead code (#6436)
  Fixed entry duplication on file download (#6437)
  Add workaround for buildSrc issue (#6433)
  Remove dash from illegal characters. (#6300)
  Docs: For developers: New architectural decision added to list (#6397)
  Added a download checkbox to the import dialog (#6381)
  Bump jaxb-xjc from 2.3.2 to 2.3.3 (#6410)
  Bump flexmark-ext-gfm-strikethrough from 0.61.20 to 0.61.24 (#6413)
  Bump byte-buddy-parent from 1.10.9 to 1.10.10 (#6408)
  Bump flexmark-ext-gfm-tasklist from 0.61.20 to 0.61.24 (#6412)
  Bump org.beryx.jlink from 2.17.8 to 2.18.0 (#6411)
  Bump tika-core from 1.24 to 1.24.1 (#6409)
  Bump flexmark from 0.61.20 to 0.61.24 (#6416)
  ...
Siedlerchr added a commit that referenced this pull request May 9, 2020
* upstream/master:
  Make wrap fields also visible in entry editor (#6315)
  Add launcher to fix extension import in snap (#6439)
  add concise message when SaveException happen (#6444)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

manually defined multiline fields in entry editor do not work
4 participants