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

Truncates the link and/or the link description in the column "linked files" in main table, if too long #6179

Merged
merged 17 commits into from
May 17, 2020

Conversation

systemoperator
Copy link
Contributor

@systemoperator systemoperator commented Mar 26, 2020

references: #6178

  • 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 bigger UI changes)
  • Checked documentation: Is the information available and up to date? If not: Issue created at https://github.com/JabRef/user-documentation/issues.

@tobiasdiez
Copy link
Member

Thanks for your PR. As this is a display issue, I would prefer if this could be fixed on the GUI side (i.e. no changes in the logic package).

One option would be to use the built-in javafx facilities and put the text in a Labeled control and use the textOverrun https://docs.oracle.com/javase/8/javafx/api/javafx/scene/control/Labeled.html#textOverrunProperty

@systemoperator
Copy link
Contributor Author

The first argument in the constructor of the java-fx MenuItem requires a String:

MenuItem menuItem = new MenuItem(linkedFileViewModel.getDescriptionAndLink(), linkedFileViewModel.getTypeIcon().getGraphicNode());

So unfortunately my hands are tied here.

@Siedlerchr
Copy link
Member

You can set the text overrun property also on the Menu item: https://docs.oracle.com/javase/8/javafx/api/javafx/scene/control/Labeled.html#textOverrunProperty

@systemoperator
Copy link
Contributor Author

I could not find out a way how to adapt it using the MenuItem. (Both links redirect to the same web page.) Suggestions are welcome.

@tobiasdiez
Copy link
Member

The labeled is part of the menuitem and thus cannot be accessed directly from code. However, it's possible to use css similar to here: https://stackoverflow.com/questions/44087291/how-to-set-context-menu-width-to-match-choice-box

@systemoperator
Copy link
Contributor Author

systemoperator commented Mar 30, 2020

Not all works as preferred, but at least it is better than without it. -fx-pref-width makes the item bigger as needed (but at least it works). -fx-max-width and -fx-text-overrun: center-ellipses would be preferred but it does not work. -fx-wrap-text: true does not work either. It's slightly frustrating, since it should have been a quick fix. Feel free to suggest any solution.

@tobiasdiez
Copy link
Member

For me adding

.menu-item > .label {
    -fx-pref-width: 100px;
}

in base.css worked. (This of course has to be further specialized as this changes every menu item).
image

@tobiasdiez tobiasdiez added the status: changes required Pull requests that are not yet complete label Apr 10, 2020
@systemoperator
Copy link
Contributor Author

-fx-pref-width already works the way it is implemented so far. In my opinion 100px is too small, because hardly anything can be read. In the combination with description and url this gets even worse. Furthermore, this OverrunStyle.CENTER_ELLIPSIS thingy (or any other customized OverrunStyle) does not work as expected. I personally, prefer my initial commit, where both, the description AND the url get centered ellipsis (since this shows the relevant parts in a perfect way) and the context menu's width is only as big as needed but limited as well. Thus, this is what I currently use for my local JabRef version.^^ How about moving the truncation code from the first commit to the location where there is now this -fx-pref-width?

@tobiasdiez
Copy link
Member

I agree, 100px is too small. I just wanted to show you how pref-width + ellipses do work.

Truncating the link and the description separately is indeed a good suggestion. Should be able by putting them in separate labels (with pref width + elipse overflow).

@tobiasdiez tobiasdiez marked this pull request as draft April 17, 2020 16:08
@koppor
Copy link
Member

koppor commented May 12, 2020

@systemoperator ping 😇

@systemoperator
Copy link
Contributor Author

systemoperator commented May 13, 2020

The current implementation has the following flaws:

  • Opening the context menu, where several files/links are associated: Even if the texts are very short, the width of the context menu has the defined max. width. -fx-max-width does not work at all, so -fx-pref-width has been used, but it always sets the context menu to the defined max. width (and wrapping the text does not work either).
    • Preference: The context menu should only be as big as needed (otherwise it's irritating and disturbing); I have tried to fix it, but I did not succeed. It seems to be a bug.
  • If the text in the context menu exceeds the defined max. width, then it currently gets truncated at the end of the text with (right) ellipses ("..."), even though I have declared, that centered ellipses (-fx-text-overrun: center-ellipses) should be used. Probably, this is another bug.
    • Preference:
      • Centered ellipses should be shown:
        • Descriptive text of the file (.../dfdfdf/asdas/file.html)
      • If this works, another improvement would be, using centered ellipses for both the descriptive text and for the path/link:
        • Descriptive ... file (https://www.yzd/.../file.html)

My initial commit truncated the text itself and added centered ellipses, which would eliminate the aforementioned second flaw. Furthermore, there was no need to define a max width, since the text was truncated and thus also the context menu itself did not exceed a specific width and was only as wide as required by the text. As a result it would have eliminated all flaws mentioned above.

Considering all the issues concerning the java-fx components, I would tend to a solution which simply truncates the texts itself (as already suggested in earlier comments above). This is, what I currently use for my local JabRef instance.

@tobiasdiez
Copy link
Member

Okay, you've convinced me. It's a bit unfortunate that there is no css-based fix, because I really think that truncating should be handled by the JavaFX controls. But well, if it's not possible then yeah...

So I'm fine with changing the display text itself as it was done in one of your earliest commits. Please add a helper method to ControlHelper https://github.com/JabRef/jabref/blob/master/src/main/java/org/jabref/gui/util/ControlHelper.java that does the actually truncation and can be reused. A bit similar to https://github.com/openjdk/jfx/blob/7b0619004af2e2d1b1a32084ef92ff5cd3880900/modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/skin/Utils.java#L218 which might also be helpful for the implementation. Thanks!

@systemoperator
Copy link
Contributor Author

@tobiasdiez What do you think about it?

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.

Thanks! Looks good to me.

Edit: the checkstyle currently fails, that needs to be fixed before merge.

@tobiasdiez tobiasdiez marked this pull request as ready for review May 16, 2020 10:44
@tobiasdiez tobiasdiez added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed status: changes required Pull requests that are not yet complete labels May 16, 2020
Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

Got a comment about a comment. 😁
Everything else looks very good. Thank you very much.

@systemoperator systemoperator force-pushed the truncate-links-and-descriptions branch from 6ebcb3c to 92e241f Compare May 17, 2020 23:01
@calixtus
Copy link
Member

Merging now. Thank you very much.

@calixtus calixtus merged commit a080191 into JabRef:master May 17, 2020
Siedlerchr added a commit that referenced this pull request Jul 1, 2020
# By dependabot-preview[bot] (18) and others
# Via GitHub (17) and others
* upstream/master: (77 commits)
  Reenable caching of gradle
  Refactor BibtexKeyPatternPreferences (#6489)
  Update CHANGELOG.md
  Add changelog entry and remove unnecessary code
  EasyBind revision part two
  Fix Drag and Drop on empty database
  Truncates DOIs and URLs in the column "Linked identifiers" in main table, if too long (#6498)
  Bump flexmark-ext-gfm-tasklist from 0.61.26 to 0.61.30
  Bump flexmark from 0.61.26 to 0.61.30
  Bump xmlunit-matchers from 2.6.4 to 2.7.0
  Bump java-string-similarity from 1.2.1 to 2.0.0
  Bump flexmark-ext-gfm-strikethrough from 0.61.26 to 0.61.30
  Bump xmlunit-core from 2.6.4 to 2.7.0
  Truncates the link and/or the link description in the column "linked files" in main table, if too long (#6179)
  Keep group pane size when resizing window (#6180) (#6423)
  Changelog: Fix missing citation for biblatex-mla
  Update AUTHORS
  Check duplicate DOI (#6333)
  Fix missing citation for biblatex-mla
  Change EasyBind dependency (#6480)
  ...

# Conflicts:
#	src/main/java/org/jabref/gui/actions/ActionHelper.java
#	src/main/java/org/jabref/gui/customentrytypes/CustomEntryTypeDialogViewModel.java
#	src/main/java/org/jabref/gui/customentrytypes/CustomizeEntryTypeDialogView.java
#	src/main/java/org/jabref/model/entry/field/FieldFactory.java
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.

5 participants