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

Rework entry editor toolbar in JavaFX #3427

Merged
merged 12 commits into from
Dec 28, 2017
Merged

Rework entry editor toolbar in JavaFX #3427

merged 12 commits into from
Dec 28, 2017

Conversation

tobiasdiez
Copy link
Member

@tobiasdiez tobiasdiez commented Nov 11, 2017

Since quite some time passed since the last issue was created concerning the entry editor, I feel like this component is getting way to stable and no longer annoys users. Thinking hard about how to reintroduce the necessary amount of bugs, I hereby propose to rework the entry editor toolbar using JavaFX.

Before:
image

After:
image

I tried to clean-up the ui as much as possible by removing a few buttons and changing the size/space.
The following buttons were removed:

  • all buttons that do not have a global effect on the whole entry but are concerned with a specific field
    • "generate bibtex key" moved to the key field in the entry editor
    • "find linked files" removed completely (it was not working anyway, and nobody complained)
    • "write metadata to pdf" moved to file field
  • "help": removed completely (since otherwise there was not enough space to have a nice display of the type label)

Other changes:

  • left click on type label shows "change type menu"
  • the entry editor contained some (unused) code that checked the bibtex key for duplication and validity. I extracted the code and turned it into new integrity checkers.

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)
  • If you changed the localization: Did you run gradle localizationUpdate?

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Nov 11, 2017
@Siedlerchr
Copy link
Member

To be honest I do not really use that left toolbar, but in general I appreciate the layout. I typically use generate bibtex key on the main table

@lenhard
Copy link
Member

lenhard commented Nov 17, 2017

Your comment about the bugs in the entry editor would be funny if I wouldn't suspect that you are actually serious about that ;-)

This PR is the logical next step in the JavaFX migration. Most of the entry editor is in FX now, so this should be as well. Your arguments regarding button placement are reasonable and you have my Go for most of it. I think the XMP-generation button is fine at the file field, but maybe @koppor (who actually uses the feature) disagrees and wants to keep it? Your main challenge will probably be to make sure that the file field does not get too cramped with all the buttons.

Personally, I don't see why the help button should go. Does it make the toolbar so much more ugly?

There's one thing with the buttons in all of the entry editor that you could address here: The button icons do not follow the color scheme of JabRef, but are always black. Could you align that to the color scheme?

Code-wise, the PR is fine. The best part is probably that the UndoManager seems to make a come back in several places. My only concern is performance (since we just repaired this). I know you like FXML files, but adding BibtexKeyEditor.fxml means that it will be loaded every time the required fields tab is rebuilt, right? Isn't that a potential problem?

@mlep
Copy link
Contributor

mlep commented Nov 20, 2017

  • About the "find linked files" button was removed completely (it was not working anyway, and nobody complained): it worked in version 4.0 (that may explain the lack of complains ;-) ). Could it be moved to the file field?
  • I feel like the need to do a right click on the entry type to change the type (e.g. article --> book) is not intuitive and not consistent: I cannot find another example of this behaviour in JabRef. A suggestion: same look and behavior as the "insert entry" button (in the main menu), i.e. a "little v" next to it, and an activation by a left click.
    Just my 2 cents.

@tobiasdiez tobiasdiez removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Nov 22, 2017
@tobiasdiez
Copy link
Member Author

I finally had time to continue working on it today. I updated the description above and the PR is now officially ready for your opinion.

@lenhard Personally, I'm in favor of keeping the buttons black.

@lenhard With the refactoring, two fxml files were introduced: one for the whole entry editor and a second for the bibtex key field. The first one is loaded at the very start of jabref and the second every time the required fields tab is shown. Admittedly, parsing and instantiating fxml files has an overhead; but it should be negliable as long as you don't load to many fxml files at the same time (e.g. like last time where every editor field was in a separate fxml and a refresh loaded >100 of them). I didn't noticed any performance-degradation with this PR.

@mlep Good points! The "change entry type" pop-up is now shown on a normal right click.

@tobiasdiez tobiasdiez added this to the v4.2 milestone Dec 2, 2017
@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 2, 2017
@koppor koppor changed the title [WIP] Rework entry editor toolbar in JavaFX Rework entry editor toolbar in JavaFX Dec 5, 2017
@tobiasdiez
Copy link
Member Author

Since this PR was now ready for review for nearly one month, I'll merge it. If you have further remarks concerning the code and the changes, I'll of course revisit the PR and implement your feedback.

@tobiasdiez tobiasdiez merged commit 1172ed4 into master Dec 28, 2017
@tobiasdiez tobiasdiez deleted the javafxEditorToolbar branch December 28, 2017 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants