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

Right click menus should allow copying doi url #2040

Merged
merged 3 commits into from Sep 26, 2016

Conversation

boceckts
Copy link
Contributor

@boceckts boceckts commented Sep 22, 2016

Right clicking on the doi field in the main table or the entry editor shows a sub menu where the user can copy the url of the doi to the clipboard. Refs #490

Right click menu when doi field isn't present
rightclick

Right click menu in entry editor when doi field is not empty
entryrightclick

Right click menu in entry editor when doi field is empty
emptydoi

  • Internal quality assurance
  • Add the menu to the entry editor
  • Change in CHANGELOG.md described
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef

@boceckts boceckts force-pushed the rightClickCopyDOI branch 4 times, most recently from c4d4c19 to 444b5ca Compare September 26, 2016 12:28
- Added to the special fields right click menu in the main table
- Added to the DOI field in the entry editor
@boceckts boceckts changed the title [WIP] Right click on doi shows menu to copy the link [WIP] Right click menus should allow copying doi url Sep 26, 2016
@Braunch Braunch added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed stupro-ready-for-internal-review labels Sep 26, 2016
@boceckts boceckts changed the title [WIP] Right click menus should allow copying doi url Right click menus should allow copying doi url Sep 26, 2016
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.

LGTM 👍
I only have a very small remark regarding the usage of the DOI class.

public void actionPerformed(ActionEvent e) {
identifier = component == null ? identifier : component.getText();

Optional<DOI> doiOptional = DOI.build(identifier);
Copy link
Member

Choose a reason for hiding this comment

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

This can be simplified a bit:
Optional<String> url = DOI.build(identifier).map(DOI::getURIAsASCIIString)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@tobiasdiez tobiasdiez removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 26, 2016
@tobiasdiez
Copy link
Member

Thanks!

@tobiasdiez tobiasdiez merged commit e4dec24 into JabRef:master Sep 26, 2016

@Override
public void actionPerformed(ActionEvent e) {
identifier = component == null ? identifier : component.getText();
Copy link
Member

Choose a reason for hiding this comment

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

Please split that in an if/else, because that ternary operator and the assignment in one statement make it hard to understand for non java exports, especially if you don't know about the operator precedence.

@@ -407,6 +408,9 @@ private void showIconRightClickMenu(MouseEvent e, int row, MainTableColumn colum
}
menu.add(new ExternalFileMenuItem(panel.frame(), entry, content.get(), content.get(), icon,
panel.getBibDatabaseContext(), field));
if (field == FieldName.DOI) {
Copy link
Member

Choose a reason for hiding this comment

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

Beware! Although FieldName.XXX looks like an enum, is is String constant, so I think you should better use equals

Copy link
Member

Choose a reason for hiding this comment

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

Always tricked by this equals vs == in Java....when do we finally port JabRef to the nice C# world 😄

Copy link
Member

Choose a reason for hiding this comment

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

Blame Java for no operator overloading...

boolean isDOIField = field.getFieldName().equals(FieldName.DOI);
doiMenuItem.setVisible(isDOIField);
boolean isDoiFieldEmpty = field.getText().isEmpty();
doiMenuItem.setEnabled(!isDoiFieldEmpty);
Copy link
Member

Choose a reason for hiding this comment

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

Please consider splitting that in an if/else, too, e.g. simple: if(! field.getText.IsEmpty) ...

Copy link
Member

Choose a reason for hiding this comment

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

I find his version more readable than if/else 😸

@Siedlerchr
Copy link
Member

@tobiasdiez A bit too fast. At least the equals thing should be fixed.

@boceckts
Copy link
Contributor Author

@Siedlerchr can I still push to this branch or should I create a new branch + pull request?

@Siedlerchr
Copy link
Member

Hm, not sure. Better create a new one based on master and then fix the things and we merge it in

@tobiasdiez
Copy link
Member

I fixed it in ffa3299.

@boceckts
Copy link
Contributor Author

@tobiasdiez thank you.

@boceckts boceckts deleted the rightClickCopyDOI branch October 11, 2016 14:26
zesaro pushed a commit to zesaro/jabref that referenced this pull request Nov 22, 2016
* Allow copying DOI url

- Added to the special fields right click menu in the main table
- Added to the DOI field in the entry editor

* Refactored getting the uri
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.

None yet

4 participants