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

Improve refactoring preview divider handling and tree navigation. #8089

Merged
merged 1 commit into from
Dec 28, 2024

Conversation

mbien
Copy link
Member

@mbien mbien commented Dec 23, 2024

  • scroll only vertically on prev/next navigation
  • reset divider only if left panel is below minimal size
  • share divider location memory between refactoring views

fixes #8068

@mbien mbien added Editor ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Dec 23, 2024
@mbien mbien added this to the NB25 milestone Dec 23, 2024
 - scroll only vertically on prev/next navigation
 - reset divider only if left panel is below minimal size
 - share divider location memory between refactoring views
@mbien mbien force-pushed the fix-refactoring-preview-divider branch from 7918afa to 9f420b4 Compare December 23, 2024 23:21
@mbien mbien added the UI User Interface label Dec 23, 2024
Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

Actually I don't really see a difference, but also no problem, so from my POV ok to go in if it helps. Apart from the inline comment, I would raise the question, whether it would be possible to persist the divider location into options as is done for search results.

@mbien
Copy link
Member Author

mbien commented Dec 25, 2024

Actually I don't really see a difference, but also no problem, so from my POV ok to go in if it helps

Try this:

  • run find-usages on something which has many results (like println())
  • if the left view doesn't have a horizontal scrollbar yet, move the divider until you see one
  • use next/prev button to navigate between results

NB <= 24 will move the horizontal scrollbar, with this PR applied the view is moved to the left and kept there on selection change.

image

image

I think allowing the tree to scroll horizontally on selection change is fine as default case for trees, but in this particular case it isn't very helpful. The tree is often shallow and the whole row is selectable, so locking it to the left makes sense I believe. (suggestion taken from the linked issue and mailing list)

I would raise the question, whether it would be possible to persist the divider location into options as is done for search results.

The divider memory is a bit of an afterthought, I didn't implement it at first. The reason for this is because this is essentially management code for many different refactoring views. It would have to persist it on a per-view-type manner. I am also not sure if there is even a good default or initial value. I think it is very usecase dependent and might not make sense to persist.

However, if the user runs a refactoring preview with slider position X, the second time it is run might look good with the same position from before since it is probably the same codebase. So maybe in-memory persistence is good enough / potentially better anyway?

This might look patchy but the way it is implemented:

        if (splitPane.getDividerLocation() < MIN_DIVIDER_LOCATION) {
            if (dividerPosMemory > MIN_DIVIDER_LOCATION) {
                splitPane.setDividerLocation(dividerPosMemory);
            } else {
                splitPane.setDividerLocation(0.3);
            }
        }

means that the whole logic won't be active if the concrete view is setting a width via regular layout logic - this would already move the divider at this point and the whole thing is skipped.

But maybe I am overthinking this.

Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

Lets keep this as is. With the comment I can see the difference. Thank you

@mbien mbien merged commit 8e539f8 into apache:master Dec 28, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) Editor UI User Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Minor improvements to new Usages preview panel (NB24)
2 participants