-
-
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
Fix/12351 area of preview hover #12355
base: main
Are you sure you want to change the base?
Conversation
…yed" This reverts commit 708d297.
You have changed from your other PR in this as well, please base your branchon main |
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.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
@laky241 please read my comment on your last PR: #12349 (review) |
@subhramit i have followed everything you told me to do please guide me what i have done wrong now jabref application is working perfect on my local machine |
|
Don't be too hard to newcomers @subhramit First time contributing needs a bit more guidance |
Apologies, Chris. My thoughts were - my minimum expectation from newcomers is that they at least try to understand the issue they're solving - which naturally answers which changes are to be kept or discarded from AI (here you will find many unrelated changes on hover properties, even the descriptions on both PRs are copied directly from AI). I want them to take home the essence of the problem-solving process, even if their code is AI generated. With these expectations, though, I definitely should have been softer on my approach. I am sorry for that. |
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.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
@subhramit sir, i understand your concern and agree that relying on AI in this way undermines the learning process and want to assure you that I take this feedback seriously. apologize for the inconvenience caused and hope to regain your trust as I improve. Thank you for your patience and guidance. Sir I have done the set up to my local machine with checkstyle and everything is working very well on my machine, need your guidance here |
Hey, it's alright. Also, please don't call me sir, I am a student as well who has just spent a bit more time contributing here. Sorry for my impatience. Thank you for taking it as positive feedback. First things first, you have based your changes on the branch that you were working on in your last PR, so both the changes got pushed to this PR together (check "Changed Files" tab on this PR). Rest we will follow up with reviews. |
value.filter(abbreviationRepository::isAbbreviatedName) | ||
.ifPresent(val -> messages.add(new IntegrityMessage(Localization.lang("abbreviation detected"), entry, field))); | ||
} | ||
return messages; | ||
} | ||
} | ||
}} |
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.
@Siedlerchr i expected checkstyle to complain here, but it's green. Maybe we need to add a rule.
src/main/java/org/jabref/logic/integrity/AbbreviationChecker.java
Outdated
Show resolved
Hide resolved
src/test/java/org/jabref/gui/linkedfile/LinkedFileEditDialogViewModelTest.java
Outdated
Show resolved
Hide resolved
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.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
… into fix/12351-area-of-preview-hover
@Siedlerchr @subhramit |
|
||
public MainTableTooltip(DialogService dialogService, GuiPreferences preferences, ThemeManager themeManager, TaskExecutor taskExecutor) { | ||
this.preferences = preferences; | ||
this.preview = new PreviewViewer(dialogService, preferences, themeManager, taskExecutor); | ||
this.setShowDelay(Duration.seconds(1)); | ||
|
||
this.setShowDelay(Duration.millis(100)); // 100ms delay |
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.
why do you change the delay?
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.
@@ -17,26 +18,51 @@ public class MainTableTooltip extends Tooltip { | |||
|
|||
private final PreviewViewer preview; | |||
private final GuiPreferences preferences; | |||
private final VBox tooltipContent = new VBox(); | |||
private final Label fieldValueLabel = new Label(); | |||
private final VBox tooltipContent = new VBox(); // A vertical box to hold the tooltip content |
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.
remove the comments they are just describing the code itself
please show a screenshot of how it looks like |
this.setShowDelay(Duration.millis(100)); // 100ms delay | ||
this.setHideDelay(Duration.millis(500)); // Slight delay before hiding | ||
|
||
// Add the field value label and preview area to the tooltip content |
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.
remoce the comments everywhere here, they state the obvious. Use comments only when you want to why you did something
Fixes #12351
This pr improves the entry preview tooltip in the main table by dynamically adjusting its size to fit the content and reducing the delay before it appears. The tooltip now provides a cleaner, more responsive user experience, especially for entries with long or short field values.
THE CHANGES MADE
The tooltip now resizes based on the content it displays, ensuring it isn’t unnecessarily large or too small.
Added support for wrapping long text and limiting the tooltip's maximum width and height.
The time it takes for the tooltip to appear after hovering over an entry has been reduced from 1 second to 100 milliseconds for a more responsive feel.