-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add icon + color and description to groups #2612
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codewise, there are only minor things. However, you need to remove the unused import to get rid of the checkstyle build failures. Also, I am getting these messages in the error log when playing around with JabRef:
Warning: could not get message translation for "Color" for locale en
Warning: no message translation for "Color" for locale en
I haven't spotted the origin of these warnings, but you should make them disappear.
One more UI-thing that I noticed, although I assume that it does not come from your changes here: When opening the "Add subgroup" dialog via the right-click menu, the right-click menu stays in the forefront of JabRef or any other application in my OS. It is right now in front of firefox as I am typing these words. This is somehow weird, any chance of getting rid of this?
To sum up: No major problems in this PR. Please fix the few minor code issues and then it is ready to be merged.
/* Colors */ | ||
// JabRef's default colors | ||
public static final Color DEFAULT_COLOR = JabRefPreferences.getInstance().getColor(JabRefPreferences.ICON_ENABLED_COLOR); | ||
public static final Color DEFAULT_DISABLED_COLOR = JabRefPreferences.getInstance().getColor(JabRefPreferences.ICON_DISABLED_COLOR); | ||
private static final String DEFAULT_ICON_PATH = "/images/external/red.png"; | ||
private static final Log LOGGER = LogFactory.getLog(IconTheme.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please sort this and the following two variable to a different part of the class declaration. In the current form they are commented as "default colors".
import org.jabref.model.groups.AllEntriesGroup; | ||
|
||
public class DefaultGroupsFactory { | ||
public static AllEntriesGroup getAllEntriesGroup() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a blank line after the class definition.
47a289e
to
d2784ff
Compare
Thanks for the review. I fixed all the code issues and will merge this PR now. I can reproduce the issue with the context menu and track it at #2599. |
* upstream/master: (91 commits) fixed #2613 (#2623) Add MathSciNet as ID-based fetcher (#2621) Add icon + color and description to groups (#2612) Fixed wrong logger import (#2618) Cleanup window has a scrollbar now. (#2614) Added the locale to a newly created class Move ExportComparator and CustomExportList to the correct package (only used in preferences) Fixes displaying of Mr DLib recommendations (#2616) Fix title-related key patterns in BibtexKeyPatternUtil (#2610) Remove OrdinalsToSuperscriptFormatter from recommended biblatex save actions (#2601) Update pgjdbc to new major version Update Dependenices String Similary log4j wiremock mockito Fix exception when parsing groups which contain a top level group (#2611) Add "Remove group and subgroups" option (#2587) Fix #1104: group selection is preserved under tab change Fix several File Cleanup + Rename issues (#2415) Fixed a small error in the code comments Implement #1904: filter groups (#2588) Braces checking followup (#2598) Improve braces checking (#2593) ...
The user can now specify an icon (and color) that is displayed next to the group's name. Moreover, a custom description text can be specified and will be shown on mouse-over.
Also the expanded status is now kept when switching between tabs.
Related issues:
Things to note:
gradle localizationUpdate
?