Fix for issue 4629 #5150

ffffatgoose · 2019-07-26T04:35:26Z

distinguish the user action between menu and button
reuse the Acitons class to add enum to the description of user action

davidemdot

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

davidemdot · 2019-07-26T06:20:19Z

src/main/java/org/jabref/gui/actions/JabRefAction.java

+ * 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) {


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.

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

ffffatgoose · 2019-07-26T16:28:17Z

just change the JabRefAction as your suggestion :)

calixtus · 2019-07-26T09:28:52Z

src/main/java/org/jabref/gui/actions/Actions.java

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


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

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

tobiasdiez

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

tobiasdiez · 2019-07-26T18:18:53Z

src/main/java/org/jabref/gui/actions/ActionFactory.java

@@ -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);


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

tobiasdiez · 2019-07-26T18:19:46Z

src/main/java/org/jabref/gui/actions/Actions.java

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


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

tobiasdiez · 2019-07-26T18:27:18Z

src/main/java/org/jabref/gui/actions/JabRefAction.java

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


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.

jabref/src/main/java/org/jabref/gui/JabRefFrame.java

Lines 1016 to 1022 in 7561724

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));

}

davidemdot · 2019-07-26T18:40:21Z

src/main/java/org/jabref/gui/actions/JabRefAction.java

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


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 · 2019-08-06T12:44:27Z

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 · 2019-08-09T00:49:29Z

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……

jabref/src/main/java/org/jabref/gui/JabRefFrame.java

Lines 529 to 541 in 9daa645

 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

ffffatgoose · 2019-08-09T09:48:36Z

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

Siedlerchr · 2019-08-09T13:48:38Z

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

calixtus · 2019-08-09T13:52:23Z

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 · 2019-08-09T14:16:17Z

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

ffffatgoose · 2019-08-10T01:01:56Z

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

Siedlerchr

From my point of view this looks good now.

Siedlerchr · 2019-08-18T12:23:06Z

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

stefan-kolb · 2019-08-24T14:59:28Z

@tobiasdiez Do you think this is fine to merge?

tobiasdiez · 2019-08-24T15:22:43Z

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

* 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 ...

1170300102 added 2 commits July 25, 2019 15:52

distinguish button and menu

945fa0c

reuse enum-Actions to distinguish user actions

5a81b90

ffffatgoose mentioned this pull request Jul 26, 2019

Improve telemetry collection #4629

Closed

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jul 26, 2019

davidemdot reviewed Jul 26, 2019

View reviewed changes

avoid duplicating code in JabRefAction

1863772

calixtus reviewed Jul 26, 2019

View reviewed changes

tobiasdiez requested changes Jul 26, 2019

View reviewed changes

davidemdot reviewed Jul 26, 2019

View reviewed changes

updateforcrash

69002a4

1170300102 added 3 commits August 9, 2019 10:02

fixExceptClassActions

88dbc74

correctCheckstyle

f5e20ad

try01

2c71b13

ffffatgoose force-pushed the fix-for-issue-4629 branch from 0761dd1 to 2c71b13 Compare August 9, 2019 09:39

tryforcheck

92b007e

Siedlerchr approved these changes Aug 18, 2019

View reviewed changes

tobiasdiez merged commit c6ded5d into JabRef:master Aug 24, 2019

koppor mentioned this pull request Feb 24, 2022

Add initial telemetry development documentation #8530

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for issue 4629 #5150

Fix for issue 4629 #5150

ffffatgoose commented Jul 26, 2019 •

edited by stefan-kolb

Loading

davidemdot left a comment

davidemdot Jul 26, 2019

Siedlerchr Jul 26, 2019

ffffatgoose commented Jul 26, 2019

calixtus Jul 26, 2019

tobiasdiez Jul 26, 2019

tobiasdiez left a comment

tobiasdiez Jul 26, 2019

tobiasdiez Jul 26, 2019

tobiasdiez Jul 26, 2019

davidemdot Jul 26, 2019

LinusDietz commented Aug 6, 2019

ffffatgoose commented Aug 9, 2019

ffffatgoose commented Aug 9, 2019

Siedlerchr commented Aug 9, 2019

calixtus commented Aug 9, 2019 •

edited

Loading

Siedlerchr commented Aug 9, 2019

ffffatgoose commented Aug 10, 2019

Siedlerchr left a comment

Siedlerchr commented Aug 18, 2019

stefan-kolb commented Aug 24, 2019

tobiasdiez commented Aug 24, 2019

	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));
	}

Fix for issue 4629 #5150

Fix for issue 4629 #5150

Conversation

ffffatgoose commented Jul 26, 2019 • edited by stefan-kolb Loading

davidemdot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffffatgoose commented Jul 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasdiez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LinusDietz commented Aug 6, 2019

ffffatgoose commented Aug 9, 2019

ffffatgoose commented Aug 9, 2019

Siedlerchr commented Aug 9, 2019

calixtus commented Aug 9, 2019 • edited Loading

Siedlerchr commented Aug 9, 2019

ffffatgoose commented Aug 10, 2019

Siedlerchr left a comment

Choose a reason for hiding this comment

Siedlerchr commented Aug 18, 2019

stefan-kolb commented Aug 24, 2019

tobiasdiez commented Aug 24, 2019

ffffatgoose commented Jul 26, 2019 •

edited by stefan-kolb

Loading

calixtus commented Aug 9, 2019 •

edited

Loading