jcryptool / crypto

JCrypTool Crypto Plug-ins
https://www.cryptool.org
Eclipse Public License 1.0
68 stars 39 forks source link

Scrolling in nested scrollable widgets #305

Closed simlei closed 4 years ago

simlei commented 4 years ago

Make views with a ScrolledComposite scroll even when the mouse pointer is located over an inner control which theoretically also offers scrolling -- but has reached the limit of the scrolled area already.

simlei commented 4 years ago

Minimal working example with clean code: nestedscroll0.7.zip

Peek 2020-05-22 15-25

grthor commented 4 years ago

I tested it under Win 10 and unfortunately it does not work as well as it does on your .gif. When I drag the mouse to the inner text field and then move the mouse wheel down, it scrolls the inner text field, but at the same time it scrolls the outer scrolled composite. In this situation only the inner textfield shoulb be scrolled.

scroller

simlei commented 4 years ago

Oof, then we're dealing with different behavior over different platforms. I think the zipped MVP of the scrolling behavior is ideal to iron out the differences between linux and windows iteratively. Could you try to adapt the code so that it works under Windows?

When that's done, I would like to see it in git to better track the differences. I think the best place would be the tests repo. I think we'll be best able to figure out how to approach it if we have two working versions for win and and linux/gtk, sharing a common code base.

What I'd try first is, to see first is what happens if you make the shell and the inner scrollable textfield bigger. Call it wishful thinking, but maybe it's just an issue with the code determining how much space is left for scrolling which would be easy to fix.

Could you also check the following because I think it matters: Does the mouse wheel fire TraverseEvents (addTraverseListener)? because this event type is designed for stuff that can propagate up the SWT tree. But under linux, it seemed that the mouse wheel (e.g. in contrast to arrow keys) never fires this event, even though it seems to me a perfect candidate to do so. I imagine, under Windows, it could be different. So, I'd try listening to that event type and see if the mouse wheel gets caught. If so, set e.doit = false and see what happens.

grthor commented 4 years ago

The problem is fixed, seems to works smooth now. The new version shoulb be tested on linux.

The problem was the check if a scrollbar reached a maximum (top or bottom). This is fixed with the following code:

public static boolean isScrollEventWithoutEffectHere(Control control, ScrollBar innerScrollableBar, MouseEvent e) {

    int direction = e.count < 0 ? ScrollableControl.DOWN : ScrollableControl.UP; 

    int minimalPos = innerScrollableBar.getMinimum();
    int currentPos = innerScrollableBar.getSelection();
    int maximalPos = innerScrollableBar.getMaximum();
    int thumbSize = innerScrollableBar.getThumb();

    if (direction == ScrollableControl.UP && currentPos == minimalPos) {
        return true;
    }
    if (direction == ScrollableControl.DOWN && currentPos + thumbSize == maximalPos) {
        return true;
    }
    return false;
}

Here is a link to the code: https://github.com/jcryptool/tests/blob/977314edff54a0080a685b9b51a4d2621420786d/org.jcryptool.test.nestedscroll/src/org/jcryptool/test/nestedscroll/ScrollingUtils.java#L142-L161

I pushed the nestedscroll project to the jcryptool test repository https://github.com/jcryptool/tests/tree/develop/org.jcryptool.test.nestedscroll.

We can work on the project in this repository.

grthor commented 4 years ago

Here a demo on how it looks now. scroller

simlei commented 4 years ago

No further change needed for this minimal working example under linux :partying_face: Seems like the text field size was, by accident, accomodating my inaccurate code, which you fixed. I just changed 2x == into <= and >=, just to be more sure that nothing slips through.

Did not know about .thumbSize() being relevant. Well I guess after looking into this problem as long as you did, you had all the scrollbar tricks figured out :)

https://github.com/jcryptool/tests/commit/28e93eea33f1e87aac8ccd37030da26c29e09c1b

I imagine that trying this out in jcryptool.product, one would do more or less the same thing as with the widthHint fix, hooking every view's root composite and recursively applying this in the SWT control tree? I'd advise to only apply this listener to a minimal number of Controls, i.e. Text with V_SCROLL, and ScrolledComposite itself. Any other scrollwheel-catching controls that come to mind?

grthor commented 4 years ago

Yes, just like the widthHint fix, add it to all top composites.

I would like to add it to all texts that have one of the SWT tags READ_ONLY, V_SCROLL and H_SCROLL. What do you think of H_SCROLL? Should it scroll right/left first and then the parent scrolled composite down/up or is that rather annoying? It would look like this: scroller

For the READ_ONLY tag, I have just adapted the code. To text fields that already have a MouseWheelListener or MouseVerticalWheelListener it should not be set, because we might overwrite an existing one.

I would like to move the code to the package org.jcryptool.core.util.ui.auto. That way we keep the "automatic" improvements collected in one place.

Here you can view my changes: https://github.com/jcryptool/tests/commit/35ed9a8c3cb54daba34ae6a614e9c8f8ff1377f7

simlei commented 4 years ago

As for readonly, is it certain that this is necessary? Under Linux, this made no difference. But if it inhibits scrolling, check OS for Windows and include that as a special case.

H_SCROLL too, I'd say. I would not disable existing behavior (disable scrolling in horizontal direction with the mouse wheel), just enhance it (after no scrolling can happen anymore, propagate it up).

Agreed on the location in "core".

On Mon, May 25, 2020, 3:11 PM Thorben Groos notifications@github.com wrote:

Yes, just like the widthHint fix, add it to all top composites.

I would like to add it to all texts that have one of the SWT tags READ_ONLY, V_SCROLL and H_SCROLL. What do you think of H_SCROLL? Should it scroll right/left first and then the parent scrolled composite down/up or is that rather annoying? It would look like this: [image: scroller] https://user-images.githubusercontent.com/20046726/82815617-ccb7bd80-9e99-11ea-9b6e-7183ca92e18e.gif

For the READ_ONLY tag, I have just adapted the code. To text fields that already have a MouseWheelListener or MouseVerticalWheelListener it should not be set, because we might overwrite it.

I would like to move the code to the package org.jcryptool.core.util.ui.auto. That way we keep the "automatic" improvements collected in one place.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jcryptool/crypto/issues/305#issuecomment-633565084, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHP225T4E5J7OGUIKBNKPLRTJU5VANCNFSM4MPGSQMA .

grthor commented 4 years ago

Can you test it again. I modified the ScrollText.java (removed the V_SCROLL tag and commented the mouseScrollListenern out). I want to make sure that it is really just a Windows problem.

simlei commented 4 years ago

Looks like this: Peek 2020-05-25 16-42

grthor commented 4 years ago

I just wanted to move the code to org.jcryptool.core.util.ui.auto and adapt it for an easy use in JCT. To be a bit more precise, I wanted to modify the code so that a programmer no longer has to create a Mouse Listener. The new code should automatically check if a scrollbar is available and set the MouseListener automatically if necessary.

I noticed that all methods and variables are public. Does this have a special reason? If not, I would change it to private so that programmers only see the methods that are needed.

simlei commented 4 years ago

If you want to set some of them private that's fine. I myself don't worry too much about visibility of such methods any more. It's a matter of taste and convention really. I like the idea of not having to declare a listener explicitely.

grthor commented 4 years ago

Ok, so I will set them to private.

I am something like an auto-complete-power-user and since auto-complete only shows visible methods that you can use I can change it to private. If the method is private it is no longer visible and will not be shown in Auto-complete.

grthor commented 4 years ago

I moved the ScrollingUtilsto org.jcryptool.core.util.ui.auto and implemented it in the Ant Colony Optimization Plugin. (It is in todays WB, try it out!)

It works like a charm. Just added SmoothScroller.scrollWidgetSmooth(scrollComposite); to the top ScrolledComposite of the its view, nothing else is needed. No strange behaviour, no errors, just pure scrolling joy 🎉

The nexts steps are:

  1. Taking a look with @simlei on the code and try to improve it. Since we add a ton of MouseWheelListeners the code should be performant and I assume that there is space for improvement, especially in the new code connecting ScrollingUtils with Smooth Scroller.
  2. Adding SmoothScroller.scrollWidgetSmooth('topScrolledComposite'); to all plugins. This will take a day maybe.

This is a big step towards a smoother user experience. Thanks again to @simlei for the good ScrollingUtils code.

grthor commented 4 years ago

Oh and here is an example how it looks like. Look at the lower part of the plugin with the description. This is a Text with SWT.READ_ONLY which avoids scrolling the scrollbar on the right. With SmoothScroller enabled the scrollbar scrolls when the mouse is pointed on the text.

Without SmoothScroller: withoutSmoothScroller

And with SmoothScroller: withSmoothScroller

grthor commented 4 years ago

The code is ready and works. I will document the code and add it to all plugins. This will issue will be closed in the next days.

grthor commented 4 years ago

All visualizations use now the SmoothScroller.

This are the related commits:

Tomorrow I add it to all analysis

grthor commented 4 years ago

All analysis plugins now use the SmoothScroller: https://github.com/jcryptool/crypto/commit/18a673c085f0825e401f819c322168952dd88887 All games too: https://github.com/jcryptool/crypto/commit/ebf00126547267f01da7dd413324008df2dbdc22

The SmoothScroller project is now finished. The tool increases the usability a lot. The code is fully documented and should not need any attention in the near future.

The Smooth Scroller is simply added via SmoothScroller.scrollSmooth(scrolledComposite); added to a ScrolledComposite