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

Delay TreeView autoWaitCursor #8275

Closed
wants to merge 1 commit into from

Conversation

lkishalmi
Copy link
Contributor

This one intends to make the annoying Wait mouse cursor to appear on every TreeView node expand.

In most cases the expand operation takes just a few ms, putting a wait cursor mouse on those operations would cause
flickering effect and also have a negative performance feeling.

It is maybe just me, though whenever I see a wait cursor, I feel that the background operation is heavy and the app needs to display that wait cursor. While I could be Ok with response time less than a second.

I think I need a separate RequestProcessor to put the showWaitCursor on as ViewUtil.uiProcessor() has just 1 executor thread behind, so when an operation would take more than 500 ms, the mouse cursor won't be changed till the execution is done.

Tested with the IDE running on this for a while. Have not seen an operation yet that needed a wait cursor yet.

@lkishalmi lkishalmi added the Platform [ci] enable platform tests (platform/*) label Feb 24, 2025
@lkishalmi lkishalmi added this to the NB26 milestone Feb 24, 2025
@lkishalmi
Copy link
Contributor Author

I wonder how can I write a test for this change, if that's even possible.

Tried to run NetBeans on my Raspberry Pi400. It is either still not slow enough to put up a wait cursor, or this patch is not working as intended. Will try to lower the DELAY for manual tests....

Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

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

lets wait with this since there is another PR which handles cursor change issue caused by navigator updates already. If this is solving the issue - lets not fix it twice. UX wise the wait cursor should only appear when the action was initiated via a click - otherwise the mental connection isn't there what the cause is. So the fix could be to not show the cursor during regular navigator updates unless the user clicks something or never show the cursor at all (which is my favorite). Since like mentioned on the other PR (#8274 (comment)), Trees have a second please-wait mechanism - the cursor already is redundant as in-progress indicator.

impl wise: The task is not cancellable so the proposed change might get stuck with the wait icon if the wrong timing hits.

@lkishalmi
Copy link
Contributor Author

Well, I do not see this one and #8274 mutually exclusive, as this one works with Projects, Files, Favorites and Services as well. While for basic CSL and Java navigators already have a wait node, and no real need for wait cursor ever.

Finally was able to test this on Raspberry Pi400 with moderate IO load browsing my directories in Favorites, seems to work.

Also RequestProcessor.Task is actually Cancellable. As far as I checked the implementation, it seems to be right, though I'm pretty sleepy by now. Maybe always calling setting the normal cursor back would be safer.

@mbien
Copy link
Member

mbien commented Feb 24, 2025

Also RequestProcessor.Task is actually Cancellable

implementing Cancellable does not magically stop a in-progress task, esp not when it has further indirection once it runs.

right now it starts in the first request processor. The first step of showWaitCursor() it enqueues another runnable on EDT. At this point it doesn't matter anymore if anything is cancelled. The implementation is not cancellable (there is no if(cancelled) return; I can find). The race would still exist even without the additional runnable btw, it would be just smaller and less likely to occur.

Well, I do not see this one and #8274 mutually exclusive, as this one works with Projects, Files, Favorites and Services as well. While for basic CSL and Java navigators already have a wait node, and no real need for wait cursor ever.

right. The wait cursor is traditionally used to communicate to the user to stop clicking ;). So I fully agree that the other PR deactivating this auto-cursor change feature is the right choice there since the cursor should not change just because the user is typing in the editor and the navigator is refreshing. And I also agree that it is redundant due to the navigator please-wait node -> great that it will be turned off there.

I have nothing against an impl which delays the cursor change on tree expand too. But lets think about the best way to implement this. We don't want to introduce "stuck-cursor" bugs due to this small cosmetic improvement. Concurrency is hard and even harder to talk about - i might post a patch later to make discussions easier.

@neilcsmith-net
Copy link
Member

neilcsmith-net commented Feb 24, 2025

Linking in https://bz.apache.org/netbeans/show_bug.cgi?id=169353 for interest.

Personally, I'd just make the default value of show wait cursor false and be done with it!

The wait cursor is traditionally used to communicate to the user to stop clicking ;)

And if something is being calculated asynchronously and non-blocking (and there is already the wait node to tell me), why am I not allowed to click on something else?!

@mbien
Copy link
Member

mbien commented Feb 24, 2025

Personally, I'd just make the default value of show wait cursor false and be done with it!

agreed, IMO off it is indeed the better default. But while toying with a different impl locally (browsing pictures on an old usb 2.0 phone which has unpredictable delays), i think it would be useful to have this functionality for favorites view / network folders etc. Impl is almost done - if it doesn't work or becomes too complex we can still throw it away ;)

@neilcsmith-net
Copy link
Member

neilcsmith-net commented Feb 24, 2025

@mbien well, bonus points if you can get it to show the right cursor for this scenario then - busy, not wait. 😄 EDIT: depending on OS - Windows looks like it calls it "working in background" - anyway, the pointer plus hourglass/timer one.

@mbien
Copy link
Member

mbien commented Feb 24, 2025

well, bonus points if you can get it to show the right cursor for this scenario then - busy, not wait.

doesn't seem to be a predefined cursor like that. Here is my attempt #8276 added @lkishalmi as co-author since it was his idea.

@neilcsmith-net
Copy link
Member

@mbien - I know, that was kind of the joke - a lot of hassle to show the wrong cursor for the problem at hand. Even more hassle to show the right one.

This was in response to your comment about showing the wait cursor to stop people clicking btw.

@lkishalmi
Copy link
Contributor Author

Closing this one in favor of #8276

@lkishalmi lkishalmi closed this Feb 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform [ci] enable platform tests (platform/*)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants