Fixes #6705 , added icon for multiple identifiers by hemantgs · Pull Request #6809 · JabRef/jabref

hemantgs · 2020-08-30T06:12:14Z

Added icon for multiple identifiers
and modified behaviour to open eprint when clicked without opening menu

Issue 1 : Icon for linked identifiers does not change when more than one entry is linked
The WEB Material design glyphicon did not have a "plural" variant so thought will change to this for singular variant

And for the plural variant used this 😄 Can change this to something else too

Issue 2 :
Made changes so that when only one entry is available the link is opened immediately without the menu

Change in CHANGELOG.md described (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

calixtus · 2020-08-30T20:04:02Z

Hi @hemantgs , thanks for your work. I will look at it the next days.

tobiasdiez

Thanks a lot for your contribution!

The code looks good to me, and I've only a few very minor remarks. Concerning the icon, I'm also not sure what to choose. There doesn't seem to be a good icon for multiple links. The database icon is already used in a few places (groups and shared database), so I'm a bit hesitant to use it here as well. What about using the link and link-variant? Other solution would be to keep the current www, and use earth for multiple links?

tobiasdiez · 2020-08-31T15:29:27Z

        icon = printedViewModel.getIcon();
        // icon.setToolTipText(printedViewModel.getLocalization());
        TABLE_ICONS.put(SpecialField.PRINTED, icon);
+


please remove this newline

removed this line

tobiasdiez · 2020-08-31T15:37:52Z

        this.setCellValueFactory(cellData -> cellData.getValue().getLinkedIdentifiers());
        new ValueTableCellFactory<BibEntryTableViewModel, Map<Field, String>>()
-                .withGraphic(this::createIdentifierGraphic)
+                .withGraphic(values -> createIdentifierGraphic(values))


The old code .withGraphic(this::createIdentifierGraphic) should still work.

reverted this

hemantgs · 2020-08-31T16:34:58Z

Thanks a lot for your contribution!

@tobiasdiez 👍 Glad to help

The code looks good to me, and I've only a few very minor remarks. Concerning the icon, I'm also not sure what to choose. There doesn't seem to be a good icon for multiple links. The database icon is already used in a few places (groups and shared database), so I'm a bit hesitant to use it here as well. What about using the link and link-variant? Other solution would be to keep the current www, and use earth for multiple links?

Yeah my initial thoughts too , Database icon is common
This is link

And this is link-variant

And earth below does not look like indicating multiple entries , I feel

calixtus

Hi @hemantgs , we looked into your work and would like to thank you for your work, which looks mostly very good.
We have an important remark about the cellfactory. Please also pay attention to the remarks of @tobiasdiez .
About the icon graphic: I personally prefer the chain for the links. But of course the question is, how to show the difference to multiple links...
Greetings from the JabCon2020!

calixtus · 2020-08-31T21:20:32Z

+        if (values.size() > 1) {
+            return cellFactory.getTableIcon(StandardField.URLS);
+        } else if (values.size() == 1) {
            return cellFactory.getTableIcon(StandardField.URL);
+        } else {
+            return null;
        }


Suggested change

if (values.size() > 1) {

return cellFactory.getTableIcon(StandardField.URLS);

} else if (values.size() == 1) {

return cellFactory.getTableIcon(StandardField.URL);

} else {

return null;

}

if (values.size() > 1) {

return IconTheme.JabRefIcons.MULTIPLE_LINKS.getGraphicNode();

} else if (values.size() == 1) {

return IconTheme.JabRefIcons.LINK.getGraphicNode();

} else {

return null;

}

We talked in JabCon2020 a bit about this and we think that it would be better to directly load the icons here instead of introducing a new field URLS in StandardFields in the model package for a ui feature.

@calixtus Yeah , that makes sense too , I have changed it to use JabRefIcons directly

I could not find a multiple links icon , so for now I have left it as a such , with link and link-variant
some other alternatives are
for single link

for multiple

calixtus · 2020-08-31T21:20:47Z

    TYPE("type", FieldProperty.TYPE),
    URI("uri", "URI"),
    URL("url", "URL", FieldProperty.EXTERNAL, FieldProperty.VERBATIM),
+    URLS("urls", "URLS", FieldProperty.VERBATIM),


removed this

calixtus · 2020-08-31T21:28:06Z

 - We fixed an issue with the Library of Congress importer.
 - We fixed the [link to the external libraries listing](https://github.com/JabRef/jabref/blob/master/external-libraries.md) in the about dialog
-
+- We fixed linked identifier icon inconsistency[#6705](https://github.com/JabRef/jabref/issues/6705)


Suggested change

- We fixed linked identifier icon inconsistency[#6705](https://github.com/JabRef/jabref/issues/6705)

- We fixed the linked identifier icon inconsistency[#6705](https://github.com/JabRef/jabref/issues/6705)

and modified behaviour to open eprint when clicked without opening menu Added change log Changed name for enum Fixed checkstyle errors

hemantgs · 2020-09-01T09:21:08Z

Greetings from the JabCon2020!

Was wondering what this was 😄 ,
Google was not helpful

Fixed checkstyle errors

calixtus · 2020-09-01T10:54:55Z

Thanks for your work @hemantgs , we took the liberty to fix some small merge errors.

* upstream/master: Enable Merging of BibDatabases (#6689) Refactor file preferences (#6779) Interrupt all running tasks during shutdown (#6118) Fixes #6705 , added icon for multiple identifiers (#6809) Apply css files correctly to dialogs (#6828) Fix link Make template more explicit

hemantgs mentioned this pull request Aug 30, 2020

Main table: linked files and identifiers: inconsistencies #6705

Closed

1 task

hemantgs force-pushed the ui-inconsistensies branch from 8fda4f7 to f8e2ed8 Compare August 30, 2020 07:36

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 30, 2020

tobiasdiez requested changes Aug 31, 2020

View reviewed changes

calixtus requested changes Aug 31, 2020

View reviewed changes

Fixes 6705 , added icon for multiple identifiers

673b9f5

and modified behaviour to open eprint when clicked without opening menu Added change log Changed name for enum Fixed checkstyle errors

hemantgs force-pushed the ui-inconsistensies branch from f8e2ed8 to 4c2f493 Compare September 1, 2020 09:10

Fixed checkstyle errors

1b373ea

Fixed checkstyle errors

hemantgs force-pushed the ui-inconsistensies branch from 4c2f493 to 1b373ea Compare September 1, 2020 09:29

tobiasdiez and others added 3 commits September 1, 2020 12:43

Remove unused icons

16ee902

Fixed merge error in CHANGELOG.md

e94e497

Merge branch 'master' into ui-inconsistensies

7273659

calixtus merged commit 4ade000 into JabRef:master Sep 1, 2020

	- We fixed linked identifier icon inconsistency[#6705](https://github.com/JabRef/jabref/issues/6705)
	- We fixed the linked identifier icon inconsistency[#6705](https://github.com/JabRef/jabref/issues/6705)

Uh oh!

Conversation

hemantgs commented Aug 30, 2020

Uh oh!

calixtus commented Aug 30, 2020

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hemantgs commented Aug 31, 2020

Uh oh!

calixtus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hemantgs commented Sep 1, 2020

Uh oh!

calixtus commented Sep 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants