Smooth Scrolling improved

[Chrriis]

I experimented with smooth scrolling and found that the existing implementation in PR #150 had several painting artifacts and scrolling issues.

I realized that:

• if conditions are met, changes in viewport position are immediately painted using blitting, thus showing the target location and then showing the timed moves.
• When pressing and not releasing page up/down, the reference location was incorrect.
• When a lightweight component is over the viewport (e.g. a tooltip), conditions are different so this has to be tested too.

This PR works for me in all cases so far (tested on Windows 10, Java 17).

The test case I use (in addition to testing on our real world application): SmoothScrollingTest.zip

[Chrriis]

Here is my updated test case:
SmoothScrollingTest.zip

Among others, it allows to change the default scroll modes to see how the smooth scrolling works.

One important point is to make sure that blit blocking is in place for the initial JViewPort.setViewPosition() but that it is restored for the timed calls. An easy way to test this is to place a conditional breakpoint that prints to the console (but returns false) in the aforementioned method in the chunk that is after canUseWindowBlitter().

[DevCharly]

Have compared the source code of this PR with branch smooth-scrolling and also tested it.

Sorry, but I'm not happy with the things you're doing and have some major concerns...

Global list of viewpanes

In class SmoothScrollingHelper, you're maintaining a global static list of all
shown JViewports in the application, which is used for temporary disabling
blitting mode.

This is problematic because it is easy to produce memory leaks. It is hard to
make absolutely sure that viewports are removed from that list. This may work as
long as all events are fired/processed properly, but if some exception is thrown
in that process, then the whole dialog/frame may stay in memory...

Using weak references could help, but actually I'm not sure whether I like to
have such a static list of all shown viewports in FlatLaf...

Disabling blitting mode of all viewports

In class SmoothScrollingHelper, you're adding a global event listener to the
toolkit, which temporary disables blitting mode in all shown viewports on all
mouse and key events. To re-enable blitting mode, you're using
SwingUtilities.invokeLater().

This actually means that nearly all application code (triggered by mouse or key
events), runs with disabled blitting mode for all shown viewports. This could
cause problems for applications that need to modify viewport scroll mode
themselves.

This also disables blitting for scrolling via trackpad or Magic
Mouse, which could make scrolling slower in that case.

I think disabling blitting mode, when needed, for a single viewport for a
very short time, to execute some specific scrolling code, is OK.

But always disabling blitting mode for all shown viewports on all
mouse and key events is overkill. And re-enabling it sometimes later using
invokeLater() is risky IMHO. You have no control regarding the code that might
be executed between disabling and re-enabling blitting mode...

JTree getRowCount()

Well, I think you know yourself that this is an extreme misuse of FlatTreeUI.getRowCount() 😉

FlatLaf/flatlaf-core/src/main/java/com/formdev/flatlaf/ui/FlatTreeUI.java

Lines 361 to 394 in c791cd0

	public int getRowCount( JTree tree ) {
	// When certain events are received like page up/down while smooth scrolling is in progress, incorrect row is identified.
	// For a page event, this would move at the top/bottom of the incorrect row.
	// Any of these events would get the row count, so when row count is requested, we temporarily set the scrollbar target value.
	int rowCount = super.getRowCount( tree );
	Container parent = tree.getParent();
	if ( parent instanceof JViewport ) {
	JViewport viewport = (JViewport)parent;
	// if an event is received that is not triggered by our smooth scrolling timers...
	if( SmoothScrollingHelper.isInSmoothScrolling( viewport ) ) {
	Container viewportParent = viewport.getParent();
	if( viewportParent instanceof JScrollPane ) {
	JScrollBar verticalScrollBar = ((JScrollPane)viewportParent).getVerticalScrollBar();
	if( verticalScrollBar != null ) {
	ScrollBarUI sbUi = verticalScrollBar.getUI();
	if( sbUi instanceof FlatScrollBarUI ) {
	int targetValue = ((FlatScrollBarUI)sbUi).getTargetValue();
	// ... and if there was an active timer on the vertical scrollbar of that tree...
	if( targetValue != Integer.MIN_VALUE ) {
	int value = verticalScrollBar.getValue();
	// ... then we force the value temporarily to the target value
	SmoothScrollingHelper.setScrollBarValueWithOptionalRepaint( viewport, verticalScrollBar, targetValue );
	SwingUtilities.invokeLater( () -> {
	// ... and restore the smooth scrolling current value.
	SmoothScrollingHelper.setScrollBarValueWithOptionalRepaint( viewport, verticalScrollBar, value );
	});
	}
	}
	}
	}
	}
	}
	return rowCount;
	}

This method should simply return the number of rows, but not temporary modify
the value of the vertical scrollbar (if scroll animation is in progress).

What side effects this might have is unpredictable for me...

But, I had an idea how tow solve the too slow keyboard scrolling for trees (and
also for multi-line text components), which made this hack is obsolete. 😄

The idea was to hook into JTree ActionMap and replace actions that do some
scrolling with a special action, which sets scrollbar value, disables blitting
and then invokes original action. This is implemented in commit 542e7d5

Solving blitting problem

At the moment I see 4 possible solutions to fix the blitting problem:

1. use (parts) of this PR to disable blitting on all shown viewports
2. use FlatScrollPaneUI.installSmoothScrollingDelegateActions( c, true, ... )
   to disable blitting for all actions that do some scrolling (similar to commit
   542e7d5) and also disable blitting in FlatCaret.adjustVisibility()
3. implement subclass of JViewport that does disable blitting if necessary
   (for temporary changes)
4. implement subclass of JViewport that overrides setViewPosition() and does
   not invoke super.setViewPosition() for temporary changes (before scrolling
   animation starts)

I'm currently trying to implement 4. because this avoids the temporary changes
to the view position, which also avoids blitting or any temporary scrolling.

The problem with using a JViewport subclass is that there is no
createViewport() method in BasicScrollPaneUI, which would make it easy to
use own viewport classes. Instead, this method is in JScrollPane.

It is also possible that people already use custom JViewport sublcasses in their applications.
Then FlatViewport is not used and blitting may occur. Could disable smooth scrolling for this case...

[Chrriis]

Hi Karl,

Yes, I know, I am not too happy with some of the things I had to do to make the whole thing work. But currently, this solves all the problems I identified and it is in production. I will gladly switch to a FlatLaf official version when one is ready because having to port the patched classes everytime I update FlatLaf is annoying...

One feature you did not mention (or have I misread?) is the fact that programmatically changing the view should not activate smooth scrolling unless within a user-initiated event. That user event could be from a button that is outside the scroll panes, hence the global list of viewports. Is the list garanteed to be cleared? I don't know if it is possible to block or cause exceptions in the notification of the events I am capturing.
In any case, I cannot change all the viewports by custom subclasses so the subclass approach is not practical IMO.
I thought that perhaps I could redefine all the scrolling actions (like your delegate) so that only these activate smooth scrolling but I wasn't sure how to garantee that all use cases were handled, and I don't know if all of these go through actions that can be wrapped.

But, I had an idea how tow solve the too slow keyboard scrolling for trees (and also for multi-line text components), which made this hack is obsolete

Ah! I will have to look in details at your approach then 😄

This also disables blitting for scrolling via trackpad or Magic Mouse, which could make scrolling slower in that case.

I have to investigate that test case...