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

Save actions gui #821

Merged
merged 73 commits into from
Feb 22, 2016
Merged

Save actions gui #821

merged 73 commits into from
Feb 22, 2016

Conversation

lenhard
Copy link
Member

@lenhard lenhard commented Feb 15, 2016

This is the first (functional) shot at integrating a GUI for configuring save actions. I think it is now time for feedback on two aspects:

  1. the save action feature itself
  2. the look of the GUI

Regarding 1.: Currently, any class implementing net.sf.jabref.logic.formatter.Formatter can be used as a save action and can be configured for a field of any non-null and non-empty name (so you can configure actions for your custom fields). That is quite flexible. It would be trivial to extend that to net.sf.jabref.logic.cleanup.CleanupJob. The problem with integrating cleanup actions is that a CleanupJob seems to represent a batch job of cleanups that are preconfigured for certain fields. It is generally not intended to execute a CleanupJob for a specific and selectable field. Hence, I am not sure if cleanups and save actions go well together. A downside of the current save action feature is that save actions cannot be undone (like the sorting feature).

Regarding 2.: The GUI is functional, not pretty.
save actions
It has a check box to enable or disable all actions, a set of controls for adding a new action, a list for displaying actions, and a button for deleting a selected action. Do you have suggestions for a better set of controls?

Finally, the layout of the controls definitely needs improvement and for some reason the size of the controls increases when adding an action. These things need to be fixed. Alas, layouting is not my favourite task...

I would be grateful if someone takes a look at this and provides some feedback!

Conflicts:
	src/main/java/net/sf/jabref/exporter/FileActions.java
	src/main/resources/l10n/JabRef_da.properties
	src/main/resources/l10n/JabRef_de.properties
	src/main/resources/l10n/JabRef_es.properties
	src/main/resources/l10n/JabRef_fa.properties
	src/main/resources/l10n/JabRef_fr.properties
	src/main/resources/l10n/JabRef_in.properties
	src/main/resources/l10n/JabRef_it.properties
	src/main/resources/l10n/JabRef_ja.properties
	src/main/resources/l10n/JabRef_nl.properties
	src/main/resources/l10n/JabRef_no.properties
	src/main/resources/l10n/JabRef_pt_BR.properties
	src/main/resources/l10n/JabRef_ru.properties
	src/main/resources/l10n/JabRef_tr.properties
	src/main/resources/l10n/JabRef_vi.properties
	src/main/resources/l10n/JabRef_zh.properties
@lenhard lenhard added type: feature status: waiting-for-feedback The submitter or other users need to provide more information about the issue status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed status: waiting-for-feedback The submitter or other users need to provide more information about the issue labels Feb 15, 2016
Conflicts:
	src/main/resources/l10n/JabRef_da.properties
	src/main/resources/l10n/JabRef_de.properties
	src/main/resources/l10n/JabRef_es.properties
	src/main/resources/l10n/JabRef_fa.properties
	src/main/resources/l10n/JabRef_ja.properties
	src/main/resources/l10n/JabRef_nl.properties
	src/main/resources/l10n/JabRef_ru.properties
@tobiasdiez
Copy link
Member

Very nice!
A few remarks or suggestions:

  1. For the GUI: Maybe add a line break after the checkbox, so that the controls for adding a new save actions are below the checkbox (and maybe add some indention). Write "field" in the textbox and delete it upon focus (similar to how the search box works). Decrease width of delete button. Besides that It is already prettier then the usual JabRef UI 😄
  2. I think the SaveAction and CleanupFormatter classes can be merged with FieldFormatterCleanup (all of them essentially just store a field and a formatter). Is CleanupFormatter even used? Moreover, the SaveActions.applyActionForField method is covered by FieldFormatterCleanup.cleanup. Replacing SaveAction by FieldFormatterCleanup has also the advantage that the SaveActionsPanel can be used to add arbitrary formatters as cleanup operations in the cleanup dialog.

@lenhard
Copy link
Member Author

lenhard commented Feb 19, 2016

Again, the things I stated above are implemented and, hopefully, this can soon be merged.

Just a few remarks:

  • Personally, I like the text "Add" and "Delete" better than the Icons that are displayed now, but whatever...
  • Undoing save actions after save works via the menu Edit -> Undo. For some reason the key binding CTRL + Z does not trigger an action after save. I consider this a bug that should be fixed outside of this PR.

@simonharrer
Copy link
Contributor

  • Strange side effect: if I add save actions, the save sort order is added as well. I just enabled the save actions, and this was added:
+@Comment{jabref-meta: saveOrderConfig:original;;false;;false;;false;}
+
+@Comment{jabref-meta: saveActions:enabled;
+pages[PageNumbersFormatter]
+;}
+
  • Removing all save actions and disabling it does not remove them from the file.
+@Comment{jabref-meta: saveOrderConfig:original;;false;;false;;false;}
+
+@Comment{jabref-meta: saveActions:disabled;;}
+

Even when they are enabled but have no configuration, they are stored.

+@Comment{jabref-meta: saveOrderConfig:original;;false;;false;;false;}
+
+@Comment{jabref-meta: saveActions:enabled;
+;}
+

I propose to only store them if they are really used. Otherwise, nothing should be stored in the bib file.

And the serialization format changes if save actions are enabled or disabled, as shown in the examples above.

@lenhard
Copy link
Member Author

lenhard commented Feb 19, 2016

And it's implemented.

I guess this PR will never end, will it? ;-)

@tobiasdiez
Copy link
Member

Mhhh... maybe it is better to apply the save actions not in the DatabaseWriter but in another place (savesession?). Because of separation of concerns and so... What do you think?
(I would also remove the undoable prefix in the field changes name. The field changes are apriori just a list of changes and have no ability to be reverted. The revert functionality is added by the UndoableFieldChange class)

@@ -13,9 +13,9 @@
with this program; if not, write to the Free Software Foundation, Inc.,
51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/
package net.sf.jabref.gui;
package net.sf.jabref.gui.databaseProperties;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid capitals in package names.

@tobiasdiez
Copy link
Member

Could you also please add the TrimFormatter and RemoveBracesFormatter from #853. Thanks!

@lenhard
Copy link
Member Author

lenhard commented Feb 22, 2016

Once more, the above comments are implemented and this PR is ready for review. Discussion points:

  • I added the default actions as recommend by @simonharrer
  • @tobiasdiez Regarding the relocation of the application the save actions: I am not convinced why SaveSession would be a better place for applying the save actions. Right now SaveSession is more of a storage object for information regarding a save. If we integrate the application of save actions, SaveSession would need to know about BibEntry, which it currently doesn't. Moreover, the BibDatabaseWriter also resorts the entries, so why not apply the save actions there? It also requires the application of save actions before writing. All in all, I currently could not find a better place for applying save actions, but I am still open for suggestions.
  • I took the liberty to implement a new feature: I you choose all as field name in the save actions, the selected formatter will be applied to all fields of all entries in the database. Say you want to always trim all fields, this sounded useful. However, the TrimFormatter in its current fashion does not work, since BibEntry.getField for some reason returns a trimmed version of the field content regardless of the content of the GUI. But that is a bug unrelated to this PR, which won't be fixed in this PR. As another example, lower casing all fields works just fine.

(Note: I will take care about the codacy stuff in a final step after we aggree on merging the current feature set, but not now when things might still change)

@tobiasdiez
Copy link
Member

I'm in favor of merging this in! (Especially since I want to use the feature 😃 ).

@lenhard It was more a feeling that the application of save actions might have a better home somewhere else. But your are right, the DataBaseWriter is probably the right place for now.

@simonharrer
Copy link
Contributor

About the icons: why not use the icons for adding and removing files in the Entry Editor -> General?

Otherwise this seems good to be merged.

lenhard added a commit that referenced this pull request Feb 22, 2016
@lenhard lenhard merged commit 73ee45a into master Feb 22, 2016
@lenhard lenhard deleted the save-actions-gui branch February 22, 2016 16:08
@stefan-kolb
Copy link
Member

Nice job 👍

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 type: feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants