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

Add tooltips for all entry types for #6074 #6203

Merged
merged 17 commits into from
Apr 16, 2020

Conversation

dimitra-karadima
Copy link
Contributor

@dimitra-karadima dimitra-karadima commented Mar 29, 2020

The pull request refers to issue #6074 by @systemoperator .

Change: Add tooltips in the "Select entry type" dialog. You can see the tooltip when hovering a button of an entry type.
Remaining: Add those tooltips in the left side of the entry editor and in the main table when clicking "Change entry type".

Fixes #6074

Still haven't figured it out yet how to implement the remaining stuff, so any help would be much appreciated!
And please let me know what you think of my implementation so far!

Change: Add tooltips in the "Select entry type" dialog. You can see the tooltip when hovering a button of an entry type.
Copy link
Contributor

@systemoperator systemoperator left a comment

Choose a reason for hiding this comment

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

Great work so far. 🎉 I have added some suggestions.

case Manual:
return Localization.lang("Technical documentation.");
case MastersThesis:
return Localization.lang("A Master’s thesis.");
Copy link
Contributor

Choose a reason for hiding this comment

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

some encoding problem

StandardEntryType entry = (StandardEntryType) entryType;
switch (entry) {
case Article:
return Localization.lang("An article from a journal or magazine.");
Copy link
Contributor

@systemoperator systemoperator Mar 29, 2020

Choose a reason for hiding this comment

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

I think it might be reasonable to favor the biblatex documentation over bibtex's, since bibtex is a subset of biblatex and biblatex is better documented. (Or choose the documentation depending on the active mode of the currently opened library - bibtex or biblatex.)

Copy link
Member

Choose a reason for hiding this comment

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

And please also add a comment that the description is coming from the biblatex manual, e.g. biblatex manual chapter 2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feedback, i will change the description wherever I used the bibtex documentation.

case MastersThesis:
return Localization.lang("A Master’s thesis.");
case Misc:
return Localization.lang("Use this type when nothing else fits");
Copy link
Contributor

Choose a reason for hiding this comment

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

The finalizing dot is missing. 😉

@systemoperator
Copy link
Contributor

@dimitra-karadima Right clicking an entry in the main table calls

public Menu getChangeEntryTypeMenu(BibEntry entry, BibDatabaseContext bibDatabaseContext, CountingUndoManager undoManager) {

which further calls

private void populateComplete(ObservableList<MenuItem> items, BibEntry entry, BibDatabaseContext bibDatabaseContext, CountingUndoManager undoManager) {
if (bibDatabaseContext.isBiblatexMode()) {

src/main/java/org/jabref/gui/EntryTypeView.java Outdated Show resolved Hide resolved
StandardEntryType entry = (StandardEntryType) entryType;
switch (entry) {
case Article:
return Localization.lang("An article from a journal or magazine.");
Copy link
Member

Choose a reason for hiding this comment

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

And please also add a comment that the description is coming from the biblatex manual, e.g. biblatex manual chapter 2

Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

see comment

Fix merge conflict between branch fix-for-issue-6074 and master
Add the following changes:
-tooltips' description comes from biblatex documentation
-comment about the biblatex manual
-description to the english localization file
return Localization.lang("A single-volume collection with multiple, self-contained contributions by distinct authors which have their own title. The work as a whole has no overall author but it will usually have an editor.");
}
case Conference -> {
return Localization.lang("A legacy alias for inproceedings.");
Copy link
Contributor

@systemoperator systemoperator Apr 3, 2020

Choose a reason for hiding this comment

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

@dimitra-karadima Minor suggestion: e.g. inproceedings could be renamed to InProceedings. Reasoning: For consistency reasons, entry types which are documented with a preceding @ (like @proceedings) in the descriptional text for an entry type could be written in the same way, as they are selectable in the UI of JabRef (e.g. Conference: @inproceedings -> InProceedings; MvBook: @book -> Book; MasterThesis: @thesis -> Thesis; ...)

Copy link
Contributor

@systemoperator systemoperator Apr 3, 2020

Choose a reason for hiding this comment

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

@dimitra-karadima For even better distinction between common text, you could also encapsulate those entry type phrases in ticks (e.g. 'InProceedings') or quotes (e.g. "InProceedings"), which makes it even more understandable. In the biblatex documentation this is done on the one hand with the @ symbol and formatting it as some code, but encapsulation would be really nice in this case. Then it is easy to distinguish e.g. the common text term theses from an the entry type 'Thesis' (which should reference to @thesis).

Copy link
Member

Choose a reason for hiding this comment

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

mvBook stand for multi volume book

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@systemoperator So what do you suggest better? Ticks or quotes over the preceding @ ?

Copy link
Contributor

@systemoperator systemoperator Apr 3, 2020

Choose a reason for hiding this comment

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

@dimitra-karadima Please use quotes, as they are used for describing the entry fields of an entry type as well (see: #6239).

@systemoperator
Copy link
Contributor

systemoperator commented Apr 3, 2020

@Siedlerchr I have noticed that the entry types Patent and Periodical as well as the type alias Electronic do not exist in StandardEntryType.java, but SuppPeriodical exists and other type aliases as well. (Acutally all other types, except those three, exist.) Is there a special reason for that?


String description = getDescription(entryType);
if (StringUtil.isNotBlank(description)) {
Tooltip tooltip = new Tooltip();
Copy link
Contributor

@systemoperator systemoperator Apr 3, 2020

Choose a reason for hiding this comment

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

I would recommend refactoring the tooltip creation to the following, which prevents display issues, if some text exceeds the screen width 😃:

Screen currentScreen = Screen.getPrimary();
double maxWidth = currentScreen.getBounds().getWidth();
Tooltip tooltip = new Tooltip(description);
tooltip.setMaxWidth(maxWidth * 2 / 3);
tooltip.setWrapText(true);
entryButton.setTooltip(tooltip);

@JabRef JabRef deleted a comment from codecov bot Apr 3, 2020
@Siedlerchr
Copy link
Member

@Siedlerchr I have noticed that the entry types Patent and Periodical as well as the type alias Electronic do not exist in StandardEntryType.java, but SuppPeriodical exists and other type aliases as well. (Acutally all other types, except those three, exist.) Is there a special reason for that?

Some of those types, e.g. Patent were from the IEETran Package but are now also in the official biblatex package. I don't know if those package is still in use, but I guess it was just historical reasons why they are there. Maybe @koppor can enlighten us
https://ctan.org/pkg/ieeetran?lang=de

Add the following changes:
-refactor the tooltip to prevent display issues
-check if the entry type is StandardEntryType with instance of
-use quotes to describe another entry type in the tooltip's description
case Software -> {
return Localization.lang("Computer software. The standard styles will treat this entry type as an alias for \"Misc\".");
}
case DATESET -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

@Siedlerchr There is a typo in DATESET, it should be called DATASET or even more preferrably Dataset, right?

Copy link
Member

Choose a reason for hiding this comment

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

@systemoperator Yes, that should be Dataset.

@systemoperator
Copy link
Contributor

systemoperator commented Apr 6, 2020

@systemoperator check my latest commit! And if you have any new thoughts on how to add those tooltips in the entry editor on in the main table it would be really helpful! I am new to the project and open source, in general, so I am a little bit lost!

@dimitra-karadima You can add the tooltips by adapting the method

public static MenuItem createMenuItem(EntryType type, BibEntry entry, UndoManager undoManager) {
MenuItem menuItem = new MenuItem(type.getDisplayName());
menuItem.setOnAction(event -> {
NamedCompound compound = new NamedCompound(Localization.lang("Change entry type"));
entry.setType(type)
.ifPresent(change -> compound.addEdit(new UndoableChangeType(change)));
undoManager.addEdit(compound);
});
return menuItem;
}

to something like the following minimal working example:

    public static MenuItem createMenuItem(EntryType type, BibEntry entry, UndoManager undoManager) {
        CustomMenuItem menuItem = new CustomMenuItem(new Label(type.getDisplayName()));
        menuItem.setOnAction(event -> {
            NamedCompound compound = new NamedCompound(Localization.lang("Change entry type"));
            entry.setType(type)
                 .ifPresent(change -> compound.addEdit(new UndoableChangeType(change)));
            undoManager.addEdit(compound);
        });
        Tooltip tooltip = new Tooltip("descriptional tooltip");
        Tooltip.install(menuItem.getContent(), tooltip);
        return menuItem;
    }

So you basically replace MenuItem with CustomMenuItem and configure it as shown above.

The good thing is, that this will set the tooltips for both pending cases:

  • When changing the entry type in the entry editor. (left side of the entry editor)
  • When right clicking a reference in the main table and clicking Change entry type

😄

Add the following changes:
-Add tooltips in the left side of the entry editor when you click the entry type you have already selected or the Change entry type button.
-Add tooltips in the main table when you right click and choose "Change entry type"
@dimitra-karadima
Copy link
Contributor Author

@systemoperator check my latest commit! And if you have any new thoughts on how to add those tooltips in the entry editor on in the main table it would be really helpful! I am new to the project and open source, in general, so I am a little bit lost!

@dimitra-karadima You can add the tooltips by adapting the method

public static MenuItem createMenuItem(EntryType type, BibEntry entry, UndoManager undoManager) {
MenuItem menuItem = new MenuItem(type.getDisplayName());
menuItem.setOnAction(event -> {
NamedCompound compound = new NamedCompound(Localization.lang("Change entry type"));
entry.setType(type)
.ifPresent(change -> compound.addEdit(new UndoableChangeType(change)));
undoManager.addEdit(compound);
});
return menuItem;
}

to something like the following minimal working example:

    public static MenuItem createMenuItem(EntryType type, BibEntry entry, UndoManager undoManager) {
        CustomMenuItem menuItem = new CustomMenuItem(new Label(type.getDisplayName()));
        menuItem.setOnAction(event -> {
            NamedCompound compound = new NamedCompound(Localization.lang("Change entry type"));
            entry.setType(type)
                 .ifPresent(change -> compound.addEdit(new UndoableChangeType(change)));
            undoManager.addEdit(compound);
        });
        Tooltip tooltip = new Tooltip("descriptional tooltip");
        Tooltip.install(menuItem.getContent(), tooltip);
        return menuItem;
    }

So you basically replace MenuItem with CustomMenuItem and configure it as shown above.

The good thing is, that this will set the tooltips for both pending cases:

* When changing the entry type in the entry editor. (left side of the entry editor)

* When right clicking a reference in the main table and clicking `Change entry type`

😄

@systemoperator Thanks for your help! I have added the tooltips and in the Change Entry Type. Please take a look and let me know what you think!

case Software -> {
return Localization.lang("Computer software. The standard styles will treat this entry type as an alias for \"Misc\".");
}
case DATESET -> {
Copy link
Member

Choose a reason for hiding this comment

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

It would be cool if could do a global renaming (refactor/rename) to change the type to Dataset

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Siedlerchr do you want it to be Dataset or DataSet?

Copy link
Member

Choose a reason for hiding this comment

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

Dataset

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Siedlerchr what do you think about my latest commit?

Copy link
Member

Choose a reason for hiding this comment

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

LGTM now! I just merged in master and resolved the conflicts. When all relevant tests pass we can merge.

//The description is coming from biblatex manual chapter 2
//Biblatex documentation is favored over the bibtex,
//since bibtex is a subset of biblatex and biblatex is better documented.
public static String getDescription(EntryType entryType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid duplicating the method getDescription(), because this makes maintenance hard.

//The description is coming from biblatex manual chapter 2
//Biblatex documentation is favored over the bibtex,
//since bibtex is a subset of biblatex and biblatex is better documented.
public String getDescription(BibEntryType selectedType) {
Copy link
Contributor

@systemoperator systemoperator Apr 7, 2020

Choose a reason for hiding this comment

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

You could e.g. declare this method static, then you can access it in the other class as well. (Or you could create some helper class with this static method, or probably find some different location for it.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@systemoperator I have implemented your first suggestion! Please let me know what you think!

Change DATESET standard entry type to Dataset
Remove getDescription() from ChangeEntryTypeMenu.java
Copy link
Contributor

@systemoperator systemoperator left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 😃

@dimitra-karadima dimitra-karadima marked this pull request as ready for review April 10, 2020 07:58
@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 10, 2020
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. Thanks for your work!

dimitra-karadima and others added 2 commits April 14, 2020 18:32
Necessary change because the new pattern is only available preview in the compiler

Co-Authored-By: Christoph <[email protected]>
@tobiasdiez
Copy link
Member

@dimitra-karadima I wanted to merge, but some tests are still failing. Can you please have a look at them. Thanks!

Add the following change because some tests were failing.
Failing unit tests should stop.
Trying to stop failing unit tests
@dimitra-karadima
Copy link
Contributor Author

@dimitra-karadima I wanted to merge, but some tests are still failing. Can you please have a look at them. Thanks!

@tobiasdiez I tried to solve some issues but it is not very clear to me what to do in order to pass the failing checks. If you could help me in any way would be much appreciated!

@Siedlerchr
Copy link
Member

@dimitra-karadima Try to merge in the latest upstream/master branch in your branch. Taht shoudl resolve most problems

@dimitra-karadima
Copy link
Contributor Author

@dimitra-karadima Try to merge in the latest upstream/master branch in your branch. Taht shoudl resolve most problems

@Siedlerchr thanks for your input but unfortunately it didn't work. I don't know what to do because my master branch also fails these tests. Is it possible that I have done any mistake while building the project in Eclipse?

@Siedlerchr
Copy link
Member

Hm this is odd. I will test it locally.

@Siedlerchr Siedlerchr merged commit b87cd5d into JabRef:master Apr 16, 2020
@Siedlerchr
Copy link
Member

@dimitra-karadima The issue was the checkstyle configuration which has still some issues with jdk14 features (the switch expression for example). I merged your PR and updated the config to exclude your file.

@dimitra-karadima
Copy link
Contributor Author

@dimitra-karadima The issue was the checkstyle configuration which has still some issues with jdk14 features (the switch expression for example). I merged your PR and updated the config to exclude your file.

@Siedlerchr thank you so much for your help!

koppor pushed a commit that referenced this pull request Dec 17, 2022
84dba23 Update international-union-of-crystallography.csl (#6279)
13dd9e8 Update zeitschrift-fur-deutsche-philologie.csl (#6340)
d95b652 Create cahiers-mondes-anciens.csl (#6203)
ded567c Create rassegna-degli-archivi-di-stato-bibliografia-generale.csl (#6275)
124777a Create scientific-online-letters-on-the-atmosphere.csl (#6261)
3c276e7 Create american-medical-association-no-url-alphabetical.csl (#6252)
595ad95 Bump mathieudutour/github-tag-action from 6.0 to 6.1 (#6287)
7008128 Create cambridge-a (#6336)
17e930c Update norsk-apa-manual-note.csl (#6338)
b360859 Update norsk-apa-manual.csl (#6337)
f6c778e Update emerald-harvard.csl (#6335)
d6c6a16 Fix Brazilian quotes on  chicago-author-date.csl (#6317)
a1549b6 Update medizinische-hochschule-hannover.csl (#6330)
da88073 Update journal-of-the-american-college-of-cardiology.csl (#6334)
a520d8e Bump nokogiri from 1.13.9 to 1.13.10 (#6333)
ba54b44 Update royal-society-of-chemistry.csl (#6328)
1378ba7 LUSEM: Remove full stop (#6332)
9e3cf89 Create interpreting.csl (#6254)
bef74ed Create conservation-science-and-practice.csl (#6258)
9fb7eb7 Bug fixes triangle.csl (#6251)
e6112ba Update ucl-university-college-apa.csl (#6250)
6dcba3a Update engineering-technology-and-applied-science-research.csl (#6247)
00fe4a2 Create constructivist-foundations.csl (#6243)
03ad71b Create les-mondes-du-travail.csl (#6234)
a2bce86 Corrections based on author instructions (#6306)

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: 84dba23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tooltips for all entry types
4 participants