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

Auto-hide editor scrollbars #829

Closed
wants to merge 2 commits into from
Closed

Auto-hide editor scrollbars #829

wants to merge 2 commits into from

Conversation

nvartolomei
Copy link
Contributor

@nvartolomei nvartolomei commented Jul 14, 2018

Since editor.xcode.like.scrollbar seems broken, I decided to make few changes to make it at least somewhat usable.

Demo https://youtu.be/ro2A7EzjrB0 with editor.xcode.like.scrollbar enabled after change.

cc @ignatov (based on git blame)

Prior to this, `editor.xcode.like.scrollbar` would render a black
rectangle instead of error stripes. This change makes error stripes
panel ignore registry option. It is always rendered and responds to events.
@ignatov
Copy link
Contributor

ignatov commented Jul 16, 2018

@nvartolomei thanks, will check it out.

@SergeyMalenkov
Copy link

Thank you for the fix. I'm testing it locally, but I do not want to push it into master as is.
First, I think many people will ask to return old behaviour and we should provide such a way.
Second, I need to discuss suggested solution with our designer who is on vacation now.
Stay tuned!

@nvartolomei
Copy link
Contributor Author

@SergeyMalenkov old behaviour is? For me editor.xcode.like.scrollbar turns scrollbar into a black rectangle, is this intended?

screen shot 2018-07-16 at 22 47 23

To me that looked like a bug/abandoned feature, so I came up with this patch which hides editor scrollbar handle (thumb) only when this registry entry is set to true. When editor.xcode.like.scrollbar is false, this patch should not change anything, if it does than it is my fault.

@ignatov
Copy link
Contributor

ignatov commented Jul 17, 2018

@SergeyMalenkov I agree with @nvartolomei, the option is experimental and we should fix the original behavior, there's nothing to discuss with UI team.

@@ -581,8 +575,7 @@ protected boolean isDark() {

@Override
protected boolean alwaysPaintThumb() {
if (scrollbar.getOrientation() == Adjustable.VERTICAL) return !(xcodeLikeScrollbar() && EditorUtil.isRealFileEditor(myEditor));
return super.alwaysPaintThumb();
return true;

Choose a reason for hiding this comment

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

It is suspicious. Why it returns true, that mean always paint thumb?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is inside MyErrorPanel, to have event listeners bound, this method should return true.

From what I understand it is only used in code editor and only in vertical context, so scrollbar.getOrientation() == Adjustable.VERTICAL always evaluates to true, so it always returns result of return !(xcodeLikeScrollbar() && EditorUtil.isRealFileEditor(myEditor));.

Under default behaviour xcodeLikeScrollbar() evaluates to false, condition is short-circuited and negated, so it returns true.

Under editor.xcode.like.scrollbar=true this would return false and hide MyErrorPanel, disabling mouse event handlers when thumb is hidden by logic from the parent class that implements OS X scrollbar hiding behaviour.

In the case of this patch, I decided to always render MyErrorPanel thumb (which is actually invisible) and have mouse event handlers active, you can see this in action in the demo video linked in PR description where I hover cursor on error stripes.

tl;dr: alwaysPaintThumb returns whether or not to render invisible thumb for error stripes, which under all conditions should be true

Is there a case when this is not desired?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The scrollbar (actual scrollbar) handle logic is correct and respects registry entry is located in ButtonlessScrollBarUI.java.

@SergeyMalenkov
Copy link

@nvartolomei I mean 2 scenarios for scroll thumb in editor:

  1. it should be always visible (current behaviour)
  2. it should be hidden after awhile (editor.xcode.like.scrollbar=true)
    Suggested patch removes first behaviour and adds the second one.
    It does not provide an ability to switch to the needed behaviour.
    But it should fix painting when editor.xcode.like.scrollbar=true.

@ignatov We should discuss which behaviour should be used by default, at least.
And I think we are going to leave the current behaviour, because no one use editor.xcode.like.scrollbar.

@nvartolomei
Copy link
Contributor Author

@SergeyMalenkov I think you misinterpreted the code a bit. Re-compile with this patch applied and test both cases (editor.xcode.like.scrollbar=true/false) and you'll see that default behaviour is not changed, only behaviour under editor.xcode.like.scrollbar=true is changed.

@SergeyMalenkov
Copy link

Sorry. Seems that patch was applied incorrectly first time. Or full rebuild helped...
I'll test a bit and then push these changes into master.

@klikh klikh added the Merged label Jul 24, 2018
@nvartolomei nvartolomei deleted the nv/IDEA-194379-auto-hide-editor-scrollbar branch April 26, 2020 07:57
SergeyZh pushed a commit that referenced this pull request Oct 9, 2020
)

* Consider navigation element in PSI equality checks

This seems to solve issues related to psi becoming outdated.
Extension of comment by ajchun:
mplushnikov/lombok-intellij-plugin#840 (comment)

Covers issues: #821, #827, #829, #840, #842, #844, #846, #850, #853, #854, #855, #857

* Remove debug logging related to equality of PsiElements

GitOrigin-RevId: e9ad9801b15dc91b872b14add012a9261033fbc7
SergeyZh pushed a commit that referenced this pull request Oct 9, 2020


GitOrigin-RevId: 4b71f55579f7c21986df01574a22b3fa39551b17
SergeyZh pushed a commit that referenced this pull request Oct 9, 2020
The root issue was fixed in IntelliJ core, see: https://youtrack.jetbrains.com/issue/IDEA-248146
#829 #840

GitOrigin-RevId: 5693b4348b4ba41599885e0b5b8ef0e152f81cfe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants