Skip to content

Make wrap fields also visible in entry editor#6315

Merged
Siedlerchr merged 6 commits into
masterfrom
multiline
May 8, 2020
Merged

Make wrap fields also visible in entry editor#6315
Siedlerchr merged 6 commits into
masterfrom
multiline

Conversation

@Siedlerchr

@Siedlerchr Siedlerchr commented Apr 19, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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.

tobiasdiez
tobiasdiez requested changes Apr 19, 2020
View reviewed changes

@tobiasdiez tobiasdiez left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/main/java/org/jabref/model/entry/field/FieldFactory.java Outdated
Comment thread src/main/java/org/jabref/model/entry/field/FieldFactory.java
calixtus
calixtus reviewed Apr 22, 2020
View reviewed changes
Comment thread src/main/java/org/jabref/preferences/JabRefPreferences.java Outdated
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into multiline
4dab523 
* 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
change comment
7d21e06
@Siedlerchr

Siedlerchr commented Apr 24, 2020

Copy link
Copy Markdown
Member Author

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

koppor
koppor requested changes Apr 26, 2020
View reviewed changes

@koppor koppor left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/main/java/org/jabref/gui/fieldeditors/FieldEditors.java
preferences.getBoolean(JabRefPreferences.ALLOW_INTEGER_EDITION_BIBTEX));

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

@koppor koppor Apr 26, 2020

Copy link
Copy Markdown
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.

@tobiasdiez tobiasdiez Apr 26, 2020

Copy link
Copy Markdown
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.

@Siedlerchr Siedlerchr Apr 27, 2020

Copy link
Copy Markdown
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.

Comment thread src/main/java/org/jabref/gui/fieldeditors/PersonsEditor.java
textInput = isSingleLine
? new EditorTextField()
: new EditorTextArea();
textInput = isMultiLine ? new EditorTextArea() : new EditorTextField();

@koppor koppor Apr 26, 2020

Copy link
Copy Markdown
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!

@Siedlerchr
Merge remote-tracking branch 'upstream/master' into multiline
1dc4fc5 
* upstream/master:
  Backward compatibility for 4.3.1 (#6296)
  Fix Export to clipboard Dialog icon (#6345)
  Refactor EntryEditorPreferences (#6245)
@Siedlerchr
add changelog
3fdb701
@Siedlerchr
Merge branch 'master' into multiline
e99a550
@calixtus

calixtus commented May 8, 2020

Copy link
Copy Markdown
Member

Ping.
How do we proceed here?

@Siedlerchr

Siedlerchr commented May 8, 2020

Copy link
Copy Markdown
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 Siedlerchr mentioned this pull request May 8, 2020
Closed
Siedlerchr added a commit that referenced this pull request May 8, 2020
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into fixactionHelperEm…
70e8077 
…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
@Siedlerchr
Merge remote-tracking branch 'upstream/master'
de13952 
* 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)
@ilippert ilippert mentioned this pull request May 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tobiasdiez tobiasdiez tobiasdiez requested changes

@calixtus calixtus calixtus left review comments

@koppor koppor koppor requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

manually defined multiline fields in entry editor do not work

4 participants

@Siedlerchr @calixtus @koppor @tobiasdiez