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

Fix for issue 4629 #5150

Merged
merged 8 commits into from Aug 24, 2019
Merged

Fix for issue 4629 #5150

merged 8 commits into from Aug 24, 2019

Conversation

@ffffatgoose
Copy link
Contributor

ffffatgoose commented Jul 26, 2019

Fix #4629


  • distinguish the user action between menu and button
  • reuse the Acitons class to add enum to the description of user action
    图片
    图片
    图片
Copy link
Member

davidemdot left a comment

Thanks for your contribution, @ffffatgoose. I would just like to comment a point.

* especially for the track execute when the action run the same function but from different source.
* @param source is a string contains the source, for example "button"
*/
public JabRefAction(Action action, Command command, KeyBindingRepository keyBindingRepository, String source) {

This comment has been minimized.

Copy link
@davidemdot

davidemdot Jul 26, 2019

Member

To avoid duplicating code, I suggest modifying the previous method (the old one), for example:

public JabRefAction(Action action, Command command, KeyBindingRepository keyBindingRepository) {
    this(action, command, keyBindingRepository, null);
}

This way, you should check in your JabRefAction() method if source is null for tracking an execution:

setEventHandler(event -> {
    command.execute();

    if (source == null) {
        trackExecute(getActionName(action, command));
    } else {
        trackExecute(String.format("%sFrom%s", getActionName(action, command), source));
    }
});

You also can use Strings.isNullOrEmpty(source), from the com.google.common.base.Strings package. Or even an Optional<String> type.

This comment has been minimized.

Copy link
@Siedlerchr

Siedlerchr Jul 26, 2019

Contributor

We have our own StringIsNullorEmpty methd in org.jabref.model.StringUtil

@ffffatgoose

This comment has been minimized.

Copy link
Contributor Author

ffffatgoose commented Jul 26, 2019

just change the JabRefAction as your suggestion :)

@@ -3,7 +3,7 @@
/**
* Global String constants for GUI actions
*/
@Deprecated
//@Deprecated

This comment has been minimized.

Copy link
@calixtus

calixtus Jul 26, 2019

Contributor

Is there a special reason for reusing the deprecated Actions-enum? StandardActions is the future... ;-)

This comment has been minimized.

Copy link
@tobiasdiez

tobiasdiez Jul 26, 2019

Member

Yes, please leave the deprecated statement (and try to avoid using this enum).

Copy link
Member

tobiasdiez left a comment

Thanks a lot for your first contribution to JabRef! The code looks quite good already and I've only a few minor suggestions for improvement before we can merge.

src/main/java/org/jabref/gui/JabRefFrame.java Outdated Show resolved Hide resolved
@@ -74,7 +74,7 @@ private static Label getAssociatedNode(MenuItem menuItem) {
}

public MenuItem configureMenuItem(Action action, Command command, MenuItem menuItem) {
ActionUtils.configureMenuItem(new JabRefAction(action, command, keyBindingRepository), menuItem);
ActionUtils.configureMenuItem(new JabRefAction(action, command, keyBindingRepository, "Menu"), menuItem);

This comment has been minimized.

Copy link
@tobiasdiez

tobiasdiez Jul 26, 2019

Member

Could you please create a new enum for the Menu and Button constants (I don't like magic strings).

@@ -3,7 +3,7 @@
/**
* Global String constants for GUI actions
*/
@Deprecated
//@Deprecated

This comment has been minimized.

Copy link
@tobiasdiez

tobiasdiez Jul 26, 2019

Member

Yes, please leave the deprecated statement (and try to avoid using this enum).

if (StringUtil.isNullOrEmpty(source)) {
trackExecute(getActionName(action, command));
} else {
trackExecute(getActionName(action, command) + "From" + source);

This comment has been minimized.

Copy link
@tobiasdiez

tobiasdiez Jul 26, 2019

Member

The ApplicationInsights library that we use supports a special way to submit additional details (which I completely forgot until 5 mins ago): https://docs.microsoft.com/en-us/azure/azure-monitor/app/api-custom-events-metrics#properties
Could you please send the source as a property please.

private void trackOpenNewDatabase(BasePanel basePanel) {
Map<String, String> properties = new HashMap<>();
Map<String, Double> measurements = new HashMap<>();
measurements.put("NumberOfEntries", (double) basePanel.getBibDatabaseContext().getDatabase().getEntryCount());
Globals.getTelemetryClient().ifPresent(client -> client.trackEvent("OpenNewDatabase", properties, measurements));
}

return command.toString();
} else {
return command.getClass().getSimpleName();
}

This comment has been minimized.

Copy link
@davidemdot

davidemdot Jul 26, 2019

Member

I have been testing this method and there is a small bug in some EditAction cases (it shows org.jabref.gui.entryeditor.SourceTab$EditAction@56048e40FromMenu). Due to there are still many classes that use Actions and OldDatabaseCommandWrapper, it is a bit complicated to deal with all of them, but I think that something like this should work...

} else {
    String commandName = command.getClass().getSimpleName();
    if (command instanceof OldDatabaseCommandWrapper || commandName.equals("EditAction")) {
        return action.toString();
    } else {
        return commandName;
    }
}

Just a proposal, please adapt it in your own way :"D

@LinusDietz

This comment has been minimized.

Copy link
Member

LinusDietz commented Aug 6, 2019

Hey @ffffatgoose, it would be awesome if you could have a look at the comments and find some time to incorporate them in the PR or discuss them in the comments in case you disagree.

@ffffatgoose

This comment has been minimized.

Copy link
Contributor Author

ffffatgoose commented Aug 9, 2019

I feel very sorry to delay for so long. I have some question for the class Actions. Below are the codes of button creation in the main frame of GUI. As the Javadoc told, OldDatabaseCommandWrapper and Actions is deprecated, however, they are still in use……

factory.createIconButton(StandardActions.NEW_ARTICLE, new NewEntryAction(this, StandardEntryType.Article, dialogService, Globals.prefs, stateManager)),
factory.createIconButton(StandardActions.DELETE_ENTRY, new OldDatabaseCommandWrapper(Actions.DELETE, this, stateManager)),
new Separator(Orientation.VERTICAL),
factory.createIconButton(StandardActions.UNDO, new OldDatabaseCommandWrapper(Actions.UNDO, this, stateManager)),
factory.createIconButton(StandardActions.REDO, new OldDatabaseCommandWrapper(Actions.REDO, this, stateManager)),
factory.createIconButton(StandardActions.CUT, new OldDatabaseCommandWrapper(Actions.CUT, this, stateManager)),
factory.createIconButton(StandardActions.COPY, new OldDatabaseCommandWrapper(Actions.COPY, this, stateManager)),
factory.createIconButton(StandardActions.PASTE, new OldDatabaseCommandWrapper(Actions.PASTE, this, stateManager)),
new Separator(Orientation.VERTICAL),
pushToApplicationButton,
factory.createIconButton(StandardActions.GENERATE_CITE_KEYS, new OldDatabaseCommandWrapper(Actions.MAKE_KEY, this, stateManager)),
factory.createIconButton(StandardActions.CLEANUP_ENTRIES, new OldDatabaseCommandWrapper(Actions.CLEANUP, this, stateManager)),
new Separator(Orientation.VERTICAL),

The input parameter is in the form of Actions, so I reuse the class to return the exact actionname of user action. Waiting for suggestions :D

1170300102 added 3 commits Aug 9, 2019
@ffffatgoose ffffatgoose force-pushed the ffffatgoose:fix-for-issue-4629 branch from 0761dd1 to 2c71b13 Aug 9, 2019
@ffffatgoose

This comment has been minimized.

Copy link
Contributor Author

ffffatgoose commented Aug 9, 2019

I feel confused about the checkstyle error……Ask for some help
图片

@Siedlerchr

This comment has been minimized.

Copy link
Contributor

Siedlerchr commented Aug 9, 2019

@ffffatgoose This is not related to your changes, it's a problem in the master branch. #5182

@calixtus

This comment has been minimized.

Copy link
Contributor

calixtus commented Aug 9, 2019

Hi @ffffatgoose . JabRef is in the middle of converting the gui from the old swing-controls to javafx (see PR #4894 ) and also refactoring the codebase to the MVVM-pattern. This is why Actions, OldCommandWrapper etc. are deprecated, but are still used, since not everything is already refactored. This will take time.
To bring some relief from confusion, the controlsFX-library brings its own Action-Class, which is different from the StandardActions-enum and the deprecated Actions-enum. The mvvmFX-ActionClass is just the EventHandler, the StandardActions-Enum of JabRef (and the Action-Interface) includes only the information about the action (Text, Icon etc.). Since not everything is converted yet, most of the menuitems still use the OldDatabaseWrapper-Stuff.

About your checkstyle-issue: I was able to reproduce it with the vanilla-sources. I think it has something to do with the gradle-build-script and some recent updates of the junit-library. Would be probably the best to create a new issue in the issuetracker on github.

Edit: Whoopsie, while writing my comment @Siedlerchr was quicker.

@Siedlerchr

This comment has been minimized.

Copy link
Contributor

Siedlerchr commented Aug 9, 2019

@ffffatgoose @calixtus The checkstyle problem on the master should be gone now

@ffffatgoose

This comment has been minimized.

Copy link
Contributor Author

ffffatgoose commented Aug 10, 2019

get it~ I only used the class Actions in the place that the input parameter is in the form of Actions, like this:
https://github.com/ffffatgoose/jabref/blob/92b007ecaf0725932a99d5c30134ba77e53922d8/src/main/java/org/jabref/gui/actions/OldDatabaseCommandWrapper.java#L54-L57
Therefore, I think it's no use to add a new enum for it. Is there and else problem I should solve or it is necessary to change my way to solve this? TvT

Copy link
Contributor

Siedlerchr left a comment

From my point of view this looks good now.

@Siedlerchr

This comment has been minimized.

Copy link
Contributor

Siedlerchr commented Aug 18, 2019

@davidemdot Could you take a second look if this is ready?

@stefan-kolb

This comment has been minimized.

Copy link
Member

stefan-kolb commented Aug 24, 2019

@tobiasdiez Do you think this is fine to merge?

@tobiasdiez

This comment has been minimized.

Copy link
Member

tobiasdiez commented Aug 24, 2019

Yes, looks good to me! Thanks again for your contribution!

@tobiasdiez tobiasdiez merged commit c6ded5d into JabRef:master Aug 24, 2019
2 checks passed
2 checks passed
codecov/project 36.9% (+0.54%) compared to be8663a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Siedlerchr added a commit that referenced this pull request Aug 25, 2019
* upstream/master: (27 commits)
  remove wrong dependencies
  add missing dependencies
  Fix exception when adding field formatter in 'Cleanup entries' dialog
  Disables the preview cycling
  Removed unused method (#5219)
  Switch from tika-parsers to tika-core (#5217)
  Fix for issue 4629 (#5150)
  fix l10n, again
  fix l10n
  Fix checkstyle
  Removed old entrypreview and corresponding actions
  Prototype
  Add minor improvements and remove unused code
  Update CHANGELOG.md
  Update to current entry types and reformat code
  Convert Integer type to primitive
  Fix localization keys
  fix checkstyle again
  fix checkstyle erros
  Bibtexextractor
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

8 participants
You can’t perform that action at this time.