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

single line text fields #4138

Merged
merged 9 commits into from
Jul 22, 2018
Merged

Conversation

Naitoreivun
Copy link
Contributor

@Naitoreivun Naitoreivun commented Jun 18, 2018

Solution for #4126. I've used text formatter to force single line behavior. Hope it's enough :) Particular fields, that should contain only one line, are stored in immutable set.

// 7 tests have failed, but they also had failed before on master:

MrDLibImporterTest. testImportDatabaseIsHtmlSetCorrectly()
MrDLibImporterTest. testImportDatabaseIsTitleSetCorrectly()
MrDLibImporterTest. testImportDatabaseIsYearSetCorrectly()
XmpUtilReaderTest. testReadArticleDublinCoreReadRawXmp
XmpUtilReaderTest. testReadArticleDublinCoreReadXmp
XmpUtilWriterTest. testWriteMultipleBibEntries
XmpUtilWriterTest. testWriteXmp

@stefan-kolb
Copy link
Member

Can't we just change the underlying GUI component from TextArea to TextField?
Also please use plain Java for making fields immutable or final, not com.google.common.collect.ImmutableSet if possible.

@Naitoreivun
Copy link
Contributor Author

Naitoreivun commented Jun 21, 2018

@stefan-kolb how about now?

I've added EditorTextField class, similar to EditorTextArea, and extracted addToContextMenu to interface (ContextMenuAddable). Also i had to change parameter type TextArea to TextInputControl in few places in order to use classes in generic way.

@Siedlerchr
Copy link
Member

That looks good so far for me! Haven't tested it yet, but codewise it looks good, (Although the final in front of method parameter is suerpfllous

@Naitoreivun
Copy link
Contributor Author

Naitoreivun commented Jun 21, 2018

I use finals to avoid stupid bugs, like reassigning variable. With them my IDE automatically underlines such stuff. Another thing is that I've decided to make code more readable, so I've put new lines between SimpleEditor constructor parameters (because there was one long line), and now Codacy yells about that. Should I extract those parameters to another class, like SimpleEditorParameters or sth?

public static FieldEditorFX getForField(final String fieldName,
final TaskExecutor taskExecutor,
final DialogService dialogService,
final JournalAbbreviationLoader journalAbbreviationLoader,
Copy link
Member

Choose a reason for hiding this comment

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

You could extract the loader and the prefs parameter and directly pass the JounralAbbrev Repo stuff which is just passed onto the field checkers later
journalAbbreviationLoader.getRepository(journalAbbreviationPreferences)

Copy link
Contributor Author

@Naitoreivun Naitoreivun Jun 23, 2018

Choose a reason for hiding this comment

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

done; I'm just wonder if it's ok to access repository at the beginning instead of calling for it every time? I mean, is there a way, that the repo could be updated?

Copy link
Member

Choose a reason for hiding this comment

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

In principle, the user might change the journal abbreviations at any time. However, it should be fine to use the repository since the field editors are recreated when switching to the new entry.

final FieldCheckers fieldCheckers = new FieldCheckers(
databaseContext,
preferences.getFileDirectoryPreferences(),
journalAbbreviationLoader.getRepository(journalAbbreviationPreferences),
Copy link
Member

Choose a reason for hiding this comment

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

Here the abbrev repo stuff is passed

@stefan-kolb stefan-kolb requested a review from tobiasdiez June 22, 2018 07:36
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

LGTM for me! 👍 Thanks for your conribution

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.

The code looks very good! I've only two very small remarks.


FieldCheckers fieldCheckers = new FieldCheckers(databaseContext, preferences.getFileDirectoryPreferences(), journalAbbreviationLoader.getRepository(journalAbbreviationPreferences), preferences.getBoolean(JabRefPreferences.ENFORCE_LEGAL_BIBTEX_KEY));
final boolean hasSingleLine = SINGLE_LINE_FIELDS.contains(fieldName.toLowerCase());
Copy link
Member

Choose a reason for hiding this comment

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

I might be mistaken here, but isSingleLine(Field) sounds more natural to me.

public class FieldEditors {

private static final Logger LOGGER = LoggerFactory.getLogger(FieldEditors.class);

public static FieldEditorFX getForField(String fieldName, TaskExecutor taskExecutor, DialogService dialogService, JournalAbbreviationLoader journalAbbreviationLoader, JournalAbbreviationPreferences journalAbbreviationPreferences, JabRefPreferences preferences, BibDatabaseContext databaseContext, String entryType, SuggestionProviders suggestionProviders, UndoManager undoManager) {
private static final Set<String> SINGLE_LINE_FIELDS = Collections.unmodifiableSet(new HashSet<>(
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add this list two InternalBibtexFields (this class needs to be refactored at some point, but for now it is good to have anything at one place).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Sorry for delay, I will do that on Saturday.

@stefan-kolb
Copy link
Member

Ping 😄

- move SINGLE_LINE_FIELDS list to InternalBibtexFields class
- rename hasSingleLine var to isSingleLine
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.

Thanks for the follow-up!

@tobiasdiez tobiasdiez merged commit f9799d4 into JabRef:master Jul 22, 2018
Siedlerchr added a commit that referenced this pull request Jul 23, 2018
* upstream/master:
  single line text fields (#4138)
  Make it easier to rename and move files (#4200)
  Fix that swing dialogs are hidden behind main window (#4205)
  Fixed: #4166 add move & rename file (#4217)
Siedlerchr added a commit that referenced this pull request Jul 24, 2018
* upstream/master:
  Update dependencies (#4231)
  Update to latest release of richtextfx (#4213)
  Fix fetcher tests (#4216)
  single line text fields (#4138)
  Make it easier to rename and move files (#4200)
  Fix that swing dialogs are hidden behind main window (#4205)
  Fixed: #4166 add move & rename file (#4217)

# Conflicts:
#	CHANGELOG.md
Siedlerchr added a commit that referenced this pull request Jul 26, 2018
* upstream/master: (47 commits)
  Make attached files relative to the file directory (#4212)
  execute set visible in swing thread to avoid blocking
  Fix isbn chimbori test (#4234)
  Rewrite web search pane in JavaFX (#4203)
  Update dependencies (#4231)
  Update to latest release of richtextfx (#4213)
  Fix fetcher tests (#4216)
  single line text fields (#4138)
  Make it easier to rename and move files (#4200)
  Fix that swing dialogs are hidden behind main window (#4205)
  Fixed: #4166 add move & rename file (#4217)
  update gradle plugins and gradlen to 4.9
  Update dependencies
  Remove unnecessary look and feel migration (#4204)
  Revert threading fix since this sometimes lead to freezes...
  Update year
  Fix a few more threading exceptions
  Refactor
  Convert CiteSeerX fetcher to new infrastructure (#4185)
  Make global font size customizable (#4178)
  ...

# Conflicts:
#	src/main/java/org/jabref/gui/BasePanel.java
#	src/main/java/org/jabref/gui/JabRefFrame.java
#	src/main/java/org/jabref/gui/mergeentries/MergeEntries.java
#	src/main/resources/l10n/JabRef_en.properties
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.

4 participants