Implement wait cursor delay for TreeView.

[mbien]

• initial delay for wait cursor activation on tree node expansion
• AutoClosable for convenience reasons, in case this is one day
  extracted into a utility
• will disable cursor after a certain amount of time in case a task gets stuck (removed - allows simpler impl)

usage:

      try (DelayedWaitCursor cursor = new DelayedWaitCursor(rootPane)) {
          cursor.enable();
          taskOfUnpredictableLength()
      } catch (Exception e) {
          LOG.log(Level.WARNING, "task failed", e);
      }

how to test:

• find an old usb 2 android phone, ipod, dvd drive etc and plug it in (or use windows)
• add to favorites view, start browsing
• regular tree expansion should not show wait cursor, slow expansion should show cursor

alternative impl to #8275

[lkishalmi]

More robust than #8275. Thank you!

[neilcsmith-net]

It looks OK if you want to get in, although I'm still not convinced on the complexity vs necessity over disabling the default. I assume the glass pane is just for the cursor and not trapping events anywhere? Not entirely sure it wouldn't be easier to just use Timer for the delay?

[mbien]

glass pane is just for the cursor. you can still click through but the tree itself can't do tasks in parallel. It will wait till the first node expansion finishes.

Originally i thought it would shield from subsequent clicks, that is also why i added the max life span so that the UI can recover.

use Timer for the delay?

I mean it would look very similar, no? I could remove the lifespan sleep() to simplify it, would be no interrupt just a cancel check.

[neilcsmith-net]

Originally i thought it would shield from subsequent clicks,

That's what I was double checking, as delaying showing it would then have functional effects.

use Timer for the delay?

I mean it would look very similar, no?

I don't think so. I have something of a preference for using Timer where the task is solely about scheduling something on the EDT. I would have thought DelayedWaitCursor could have a Timer and be entirely controlled on the EDT? One Timer, no RP, no use of invokeLater.

Something like (off top of head!) -

cursor.start(); // starts cursor timer
ViewUtil.uiProcessor().post(() -> {
           try {
                node.getChildren().getNodesCount(true);
            } catch (Exception e) {
                LOG.log(Level.WARNING, "can't determine node count", e);
            } finally {
                EventQueue.invokeLater(cursor::hide); // stops timer and possibly reverts glass pane and cursor
            }
        });

[mbien]

usage wise: if you replace start() with enable() and hide() with disable() you can do the exact same thing with the impl of this PR.

using swing Timer and dropping the life span concept (since that would require two timers) the impl would look like:

    /// Shows the wait cursor after an initial delay.
    /// Can be used in ARM-blocks.
    private static class DelayedWaitCursor implements AutoCloseable {

        private static final int SPAWN_DELAY = 200;

        private final JRootPane root;
        private Timer timer;

        private DelayedWaitCursor(JRootPane root) {
            this.root = root;
        }

        private synchronized void enable() {
            if (timer != null) {
                return;
            }
            timer = new Timer(SPAWN_DELAY, (e) -> {
                root.getGlassPane().setCursor(Cursor.getPredefinedCursor(Cursor.WAIT_CURSOR));
                root.getGlassPane().setVisible(true);
            });
            timer.setRepeats(false);
            timer.start();
        }

        private synchronized void disable() {
            if (timer != null) {
                timer.stop();
                SwingUtilities.invokeLater(() -> {
                    root.getGlassPane().setVisible(false);
                    root.getGlassPane().setCursor(null);
                });
                timer = null;
            }
        }

        @Override
        public void close() {
            disable();
        }
    }

this would also work since the timer has a global reentrant lock for the queue. So i believe there can't be a race between stop() and wait cursor activation - which is the main concern since this leads to stuck cursors.

[neilcsmith-net]

I don't understand why you're doing anything more than this and making sure this class is only ever used on the EDT?

private static class DelayedWaitCursor {

        private static final int SPAWN_DELAY = 200;

        private final JRootPane root;
        private final Timer timer;

        private DelayedWaitCursor(JRootPane root) {
            this.root = root;
            timer = new Timer(SPAWN_DELAY, (e) -> {
                root.getGlassPane().setCursor(Cursor.getPredefinedCursor(Cursor.WAIT_CURSOR));
                root.getGlassPane().setVisible(true);
            });
            timer.setRepeats(false);
        }

        private void enable() {
            timer.start();
        }

        private void disable() {
           timer.stop();
           root.getGlassPane().setVisible(false);
           root.getGlassPane().setCursor(null);
        }
    }

[mbien]

sure it can be initialized in the constructor. it was just a remainder of the original impl. Its not obvious to me that the cursor change can be set off-edt, so I would rather keep it in IvokeLater.

[neilcsmith-net]

In what I suggested, disable() (or hide()) is only called on the EDT. My concern with what you have is in trying to make this UI class thread safe in the first place.

And if we're doing EDT-only scheduling, then I think Timer is usually the way to go - eg. in a class I was looking at recently doing something somewhat similar - https://github.com/apache/netbeans/blob/master/java/java.module.graph/src/org/netbeans/modules/java/module/graph/GraphTopComponent.java#L98

[mbien]

you get thread safety for little extra work since it already has to deal with concurrency anyway. Cursor activation can be a different thread than deactivation. Letting client use invokeLater() or the impl is not a big difference - i rather let the impl do that.

It would also be a weird lack of symmetry when enable could be called from any thread but disable not

[neilcsmith-net]

It would also be a weird lack of symmetry when enable could be called from any thread but disable not

I am literally suggesting that neither should be called from any thread but the EDT. We seem to be talking at cross purposes.

Or are you saying that tree expansion events are being sent off of the EDT?!

I prefer putting concurrency details where they're defined. The current implementation should use invokeLater in the finally block to post the cursor removal back to the EDT as it is.

[mbien]

Or are you saying that tree expansion events are being sent off of the EDT?!

I haven't even checked since its 2 threads no matter what the first thread is + any delay impl will have queue a task somehow (using technically a third scheduling thread) and run it on EDT. So it really makes no difference.

I feel we waste a lot of time discussing nothing here. Although this wasn't mentioned anywhere, I take the main conclusion is that the max lifespan concept which something which is not needed here - if this is the case we can indeed simplify the impl and even use the swing Timer as scheduler which saves a few lines. I might update the code later but I have no time to test it again atm - also I don't have access to that old android phone for a few days which made testing easy.

[mbien]

@neilcsmith-net I would like to apologize for what i wrote in the last post. It reads a bit harsh. What I meant was that by removing the max-lifetime aspect (which is very common for cursor changes, most OS launchers do this for example) we can switch to a much simpler implementation (~36 loc).

The second point was that due to the nature of having to decouple from the thread (EDT or not) in the enable() method due to the delay, the first method already is thread safe by nature. Making disable() also thread safe is just a nice-to-have without added complexity. That is why I would like to make this whole class thread safe - this also simplifies the client contract. (I also shouldn't have posted the snippet in #8276 (comment) since it contained too much stuff from the original RP based version still).

edit: the current usage in this PR uses it from one thread btw

Maybe one day we can cherry pick some of the reusable utilities and put it into a shared UI module - I think this could be a candidate.

[neilcsmith-net]

@mbien no problem, however I still don't particularly like your current implementation - I preferred your original ...

The second point was that due to the nature of having to decouple from the thread (EDT or not) in the enable() method due to the delay, the first method already is thread safe by nature.

Is it? The implementation might be thread safe, but I'm not aware that Timer methods are documented to be thread safe? I suggested Timer because it's good for situations where all control and callbacks are intended to happen on the EDT. I prefer trying to keep concurrent code isolated and together, rather than sprinkled around - ie. part of the call to the ViewUtil.uiProcessor. And any delayed UI code should not, itself, have to be thread-safe.

This is a lot of discussion for a minor change! 😄

[mbien]

I still don't particularly like your current implementation - I preferred your original

e05c956 that one? I can still revert it, it already was approved+tested which speeds things up.

The implementation might be thread safe, but I'm not aware that Timer methods are documented to be thread safe?

all queue methods are under a reentrant lock, the internal queue is accessed under a class lock. The doc does also not mention that it is not thread safe ;)

I prefer trying to keep concurrent code isolated and together, rather than sprinkled around

I don't understand this argument since everything is in one utility class.

[neilcsmith-net]

e05c956 that one? I can still revert it, it already was approved+tested which speeds things up.

Yes. I did say that this would be simpler if you moved all the scheduling code on to the EDT and used a Timer. If you want to keep controlling it off of the EDT, then stick with what you had.

The doc does also not mention that it is not thread safe ;)

It's Swing! 😉 Everything that is meant to be thread safe is stated to be documented as such. I checked the implementation already.

I prefer trying to keep concurrent code isolated and together, rather than sprinkled around

I don't understand this argument since everything is in one utility class.

I have a preference to inside one method / call approach when it can be, that's all.