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

Fix/12351 area of preview hover #12355

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

laky241
Copy link

@laky241 laky241 commented Jan 5, 2025

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.

@Siedlerchr
Copy link
Member

You have changed from your other PR in this as well, please base your branchon main

Copy link
Contributor

@github-actions github-actions bot left a 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
Copy link
Member

@laky241 please read my comment on your last PR: #12349 (review)

@subhramit subhramit closed this Jan 5, 2025
@laky241
Copy link
Author

laky241 commented Jan 5, 2025

@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

@subhramit
Copy link
Member

subhramit commented Jan 5, 2025

@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

  1. You have not set up a proper local development environment with Checkstyle.
  2. Not a single line of code in this PR is your own (including the PR description).

@Siedlerchr Siedlerchr reopened this Jan 5, 2025
@Siedlerchr
Copy link
Member

Don't be too hard to newcomers @subhramit First time contributing needs a bit more guidance

@subhramit
Copy link
Member

subhramit commented Jan 5, 2025

Don't be too hard to newcomers @subhramit First time contributing needs a bit more guidance

Apologies, Chris.
Basically, I observed both the entire PRs were completely AI generated and your review comments in the last one have also been ignored (with the same brace indentations as well). No issues if we want to spend time on this.

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.

Copy link
Contributor

@github-actions github-actions bot left a 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
Copy link
Author

laky241 commented Jan 5, 2025

@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

@subhramit
Copy link
Member

subhramit commented Jan 5, 2025

@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.

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).
Ideally what you should've done:
Change your branch to main in your local dev setup (use IntelliJ, or the terminal - git checkout main).
Then, make a fresh branch based on main again. Make changes pertaining to your target issue there, and then push and create a PR.
What you've done - made the new branch based on the last branch (without switching back to main in between).
Quick fix from here would be reverting the unrelated changes and then pushing them (on this branch itself).
Let me know if you need more elaboration on any step.

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;
}
}
}}
Copy link
Member

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.

Copy link
Contributor

@github-actions github-actions bot left a 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
Copy link
Author

laky241 commented Jan 6, 2025

@Siedlerchr @subhramit
made the requested changes, please guide me if i have done any mistake here
Thank you for all your guidance so far, I'm learning a great deal thanks to you.


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
Copy link
Member

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?

Copy link
Member

Choose a reason for hiding this comment

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

grafik

This can be discussed.
How does the tooltip behave in other applications, word for instance?

@@ -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
Copy link
Member

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

@Siedlerchr
Copy link
Member

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Area of preview on hover should be shrink to the size of the text displayed
5 participants