Showing correct icon on main table linked files column #6264

leitianjian · 2020-04-08T15:25:50Z

Hello, I have fixed the bugs that appear in the icon of the linked file column. I create a draft pull request. If there are breaking any contribution rules, please let me know. If everything is ok, I will create a pull request then.

I have tested to import some format of doc, just a simple test of .docx, .pptx and .pdf format files.

About the source code, I have a question:
I found the wrong icon still shows in the General -> Files list, which only shows pdf icon no matter what format of files attached. Then I found org.jabref.gui.fieldeditors.LinkedFileViewModel#getTypeIcon() which directly indicate the icon to pdf, which seems like the source of the bug. There is a TODO comment above, which seems like this bug has already discovered. Then I went through the issue list and tried to find some relevant thing to this bug (TODO), but nothing found. I would be happy if you can provide more info about it. Thanks, ^_^

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.

tobiasdiez

Thanks for your PR.

I think the original getName was ok there. We want to save the name of the file type which is in this case "PDF". It's more a problem with the display, which is controlled by

jabref/src/main/java/org/jabref/gui/maintable/MainTableColumnFactory.java

Lines 409 to 420 in 8968ccf

    
           private Node createFileIcon(List<LinkedFile> linkedFiles) { 
        
               if (linkedFiles.size() > 1) { 
        
                   return IconTheme.JabRefIcons.FILE_MULTIPLE.getGraphicNode(); 
        
               } else if (linkedFiles.size() == 1) { 
        
                   return externalFileTypes.fromLinkedFile(linkedFiles.get(0), false) 
        
                                           .map(ExternalFileType::getIcon) 
        
                                           .orElse(IconTheme.JabRefIcons.FILE) 
        
                                           .getGraphicNode(); 
        
               } else { 
        
                   return null; 
        
               } 
        
           }

.

The improvements around the getTypeIcon method you suggest would indeed be a valuable addition!

Finally, I think, we should convert the filetype string in LinkedFile to use the interface FileType. This would prevent the irritation if the filetype is specified by name or by extension or by something else. @leitianjian if you feel like you want to do this, please be invited to do so.

leitianjian · 2020-04-10T14:37:32Z

Thanks for your instructions!

I'm not very familiar with the code in fact. Really appreciate that you can indicate the source of the problem. I will try again.

For the addition bug (I think), I will try to work on it and thanks for your tips.

tobiasdiez · 2020-04-10T15:42:07Z

Nice to hear! You can also reuse this PR if you want (just push your updates to the same branch).

Siedlerchr · 2020-04-10T16:58:12Z

Please be aware that a "LinkedFile" can also be a http url or link. I don't know if this is actually considered in the file type.

leitianjian · 2020-04-11T02:42:47Z

Ok, I will reuse this PR next time I want to commit some code.

@Siedlerchr Thanks for your additional tips, which I have not considered yet. I will review the source code carefully.

… into pdf-icon-in-linked-files-column

leitianjian · 2020-04-15T09:57:58Z

Fixes #6169 and finish the TODO additional feature, which has mentioned before.

Hello, I have fixed the bugs that appear in the icon of the linked file column and finish the additional feature. I create a draft pull request. If there are breaking any contribution rules, please let me know. If everything is ok, I will create a pull request then.

I have tested to import some format of doc, just a simple test of .docx, .pptx and .pdf format files.

For the bugs, I found that is coming from the wrong filter. I think it should be a mistake.

For the additional features, I was using the same method in:

jabref/src/main/java/org/jabref/gui/maintable/MainTableColumnFactory.java

Lines 412 to 416 in 8968ccf

    
           } else if (linkedFiles.size() == 1) { 
        
               return externalFileTypes.fromLinkedFile(linkedFiles.get(0), false) 
        
                                       .map(ExternalFileType::getIcon) 
        
                                       .orElse(IconTheme.JabRefIcons.FILE) 
        
                                       .getGraphicNode();

I think the modification won't do any harm to other code

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.

tobiasdiez

Thanks a lot! This already looks really good. So I've marked it as ready for review for the other developers.

Please add a changelog entry, then this is good to go from my side.

tobiasdiez · 2020-04-15T10:13:10Z

src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java

@@ -140,7 +140,9 @@ public String getDescriptionAndLink() {
     * org.jabref.gui.externalfiletype.ExternalFileTypes#getExternalFileTypeByName(String)}
     */
    public JabRefIcon getTypeIcon() {
-        return IconTheme.JabRefIcons.PDF_FILE;
+        return externalFileTypes.fromLinkedFile(linkedFile, false)


Can you please also remove the above TODO

Ok, I get it. I will finish it in one hour. Many thanks for your instructions

Siedlerchr

Thank you very much for your contribution! Looks good to me as well!
Could you please add a changelog entry as well?

leitianjian · 2020-04-15T11:57:26Z

I have finished the changing of CHANGELOG.md and the deletion of TODO comments. Thanks for your instructions! @tobiasdiez @Siedlerchr

tobiasdiez · 2020-04-15T12:14:56Z

Perfect! Thanks again for your contribution and hope to see the next PR from you soon!

leitianjian · 2020-04-15T12:17:43Z

Perfect! Thanks again for your contribution and hope to see the next PR from you soon!

I'm searching for our next target issue ^_^

…ionCaseInsensitive * upstream/master: (25 commits) ActionHelper to test for present file (#6151) Reduce memory footprint (#6298) Add missing abbreviated journal names (#6292) fix l10n again fix checkstyle fix l10n Try to minimize CodeCov "wrong metrics" Showing correct icon on main table linked files column (#6264) Fix labels for outdated dependency issue Change one more line Squashed 'src/main/resources/csl-styles/' changes from c31d9ca..c1793d2 Resolve unit test from failing Add one more change Fix errors RIS import takes the wrong date and duplicates abstract (#6272) Update EntryTypeView.java Change to the old school format Fix XmpExporterTest (#6289) Add checkstyle screenshot (and lint guidelines-...md) Squashed 'src/main/resources/csl-styles/' changes from db54e56..c31d9ca ...

Showing correct icon on main table linked files column

aa3132b

tobiasdiez requested changes Apr 10, 2020

View reviewed changes

leitianjian closed this Apr 10, 2020

tobiasdiez mentioned this pull request Apr 14, 2020

":PDF" will not show the pdf icon in the #6169

Closed

1 task

leitianjian added 3 commits April 15, 2020 17:43

Showing correct icon on both main table and linked files column

9c9b7c1

Merge remote-tracking branch 'origin/pdf-icon-in-linked-files-column'…

2302b8a

… into pdf-icon-in-linked-files-column

Showing correct icon on both main table and linked file editor list

8090ea5

leitianjian reopened this Apr 15, 2020

tobiasdiez marked this pull request as ready for review April 15, 2020 10:11

tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 15, 2020

tobiasdiez requested changes Apr 15, 2020

View reviewed changes

tobiasdiez reviewed Apr 15, 2020

View reviewed changes

delete the TODO comments

72fc91c

Siedlerchr approved these changes Apr 15, 2020

View reviewed changes

leitianjian and others added 2 commits April 15, 2020 19:46

add record in CHANGELOG.md

e979664

Merge branch 'master' into pdf-icon-in-linked-files-column

a05ecca

tobiasdiez approved these changes Apr 15, 2020

View reviewed changes

tobiasdiez merged commit de7deb2 into JabRef:master Apr 15, 2020

leitianjian deleted the pdf-icon-in-linked-files-column branch April 16, 2020 10:07

leitianjian restored the pdf-icon-in-linked-files-column branch April 16, 2020 10:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Showing correct icon on main table linked files column #6264

Showing correct icon on main table linked files column #6264

leitianjian commented Apr 8, 2020

tobiasdiez left a comment

leitianjian commented Apr 10, 2020

tobiasdiez commented Apr 10, 2020

Siedlerchr commented Apr 10, 2020

leitianjian commented Apr 11, 2020

leitianjian commented Apr 15, 2020 •

edited

Loading

tobiasdiez left a comment

tobiasdiez Apr 15, 2020

leitianjian Apr 15, 2020 •

edited

Loading

Siedlerchr left a comment

leitianjian commented Apr 15, 2020

tobiasdiez commented Apr 15, 2020

leitianjian commented Apr 15, 2020

	private Node createFileIcon(List<LinkedFile> linkedFiles) {
	if (linkedFiles.size() > 1) {
	return IconTheme.JabRefIcons.FILE_MULTIPLE.getGraphicNode();
	} else if (linkedFiles.size() == 1) {
	return externalFileTypes.fromLinkedFile(linkedFiles.get(0), false)
	.map(ExternalFileType::getIcon)
	.orElse(IconTheme.JabRefIcons.FILE)
	.getGraphicNode();
	} else {
	return null;
	}
	}

Showing correct icon on main table linked files column #6264

Showing correct icon on main table linked files column #6264

Conversation

leitianjian commented Apr 8, 2020

tobiasdiez left a comment

Choose a reason for hiding this comment

leitianjian commented Apr 10, 2020

tobiasdiez commented Apr 10, 2020

Siedlerchr commented Apr 10, 2020

leitianjian commented Apr 11, 2020

leitianjian commented Apr 15, 2020 • edited Loading

tobiasdiez left a comment

Choose a reason for hiding this comment

tobiasdiez Apr 15, 2020

Choose a reason for hiding this comment

leitianjian Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

Siedlerchr left a comment

Choose a reason for hiding this comment

leitianjian commented Apr 15, 2020

tobiasdiez commented Apr 15, 2020

leitianjian commented Apr 15, 2020

leitianjian commented Apr 15, 2020 •

edited

Loading

leitianjian Apr 15, 2020 •

edited

Loading