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

There is no way to zoom on a small slice #1407

Closed
vallsv opened this issue Mar 31, 2023 · 9 comments · Fixed by #1482
Closed

There is no way to zoom on a small slice #1407

vallsv opened this issue Mar 31, 2023 · 9 comments · Fixed by #1482

Comments

@vallsv
Copy link

vallsv commented Mar 31, 2023

Sounds like the contraint to avoid zoom on click is taking the constraint on x and y independently.

As result there is no way to zoom on a small slice.

image

The 2 dimensions should be used together (distance or distance^2 or manhattan distance).

Also it would be much better to apply this constrain only at the start of the interaction.
If i create create a big rect and reduce it progressively during the interaction, the constraint should not be applied.

@axelboc
Copy link
Contributor

axelboc commented Apr 3, 2023

Did you try passing a smaller threshold with the minZoom prop? Just set a number that is small-enough for your liking. Try minZoom={2}, for instance.

IMO, there's no point in complexifying this feature with complicated logic when the goal is just to prevent zooming on click as well as to prevent zooming on a 0px^2 area.

@vallsv
Copy link
Author

vallsv commented Apr 3, 2023

That's not complex, that's just basic stuff. I talk about a slice of 2000x2

@axelboc
Copy link
Contributor

axelboc commented Jul 26, 2023

We discussed this with @t20100 and @loichuder back in April -- sorry for not following up sooner.

The conclusion was that we weren't convinced by the need to zoom on such a thin slice, and thought it was more likely to happen by mistake (i.e. mouse button released at the wrong time while drawing the selection).

Also, it seems easy-enough to reach the desired zoom level by either: drawing multiple zoom boxes in a row; or zooming once and then performing an axial zoom with the mouse wheel via a discoverable keyboard shortcut.

@axelboc axelboc closed this as completed Jul 26, 2023
@axelboc axelboc added the wontfix This will not be worked on label Jul 26, 2023
@vallsv
Copy link
Author

vallsv commented Jul 27, 2023

I am very supprised that you decide on your own what is important and what is not for users.

Especially for a very easy fix like that.

@vallsv
Copy link
Author

vallsv commented Jul 27, 2023

So i have checked the code.

Instead of using hasMinSize you can use the following and it should be done

public hasMinSize(minSize: number): boolean {
    const { width, height } = this.size;
    return width + height >= minSize;  # manhattan distance
  }

This validation is one of the most annoying interaction.
Because if you want to zoom at a place you just want to zoom at a place.
And there is no reason to cancel that.

The reason i can see is to avoid some glitches with false interaction. But when you have spend 5 seconds to move your mouse, you are not in such situation.

@axelboc
Copy link
Contributor

axelboc commented Jul 27, 2023

Okay... let's take it from the start.

As result there is no way to zoom on a small slice.

Why is this a problem? I'm arguing that it's a good thing as it avoids mistakes. Zooming on a tiny slice can be quite disorienting.

Also it would be much better to apply this constrain only at the start of the interaction.
If i create create a big rect and reduce it progressively during the interaction, the constraint should not be applied.

But want to spend 5 seconds to move your mouse, you are not in such situation.

Agreed, but this part of the issue is not a "very easy fix".

I am very surprised that you decide on your own what is important and what is not for users.

You're the one making the request, so it is up to you to build a case for it! 😝 I'm just making a judgement call with the information I've got, with my knowledge of the codebase, and after consultation with other experienced UI developers. If you're unhappy with this call, you're very welcome to provide counter-arguments. Have you actually seen real users try the interaction and become frustrated by it? Perhaps you have, but you haven't told us so and haven't described such a situation, so your word is as good as ours.

Note also that we provide an issue template for a reason. Telling maintainers what should or shouldn't be done in their project just because is rarely the best way to get what you want.


A typical way forward in this sort of situation is to give more control to the library consumer to change the default behaviour. Would it be satisfactory to you if I added a validate prop to SelectToZoom so you could override the default validation logic with your own?

@axelboc axelboc reopened this Jul 27, 2023
@axelboc axelboc removed the wontfix This will not be worked on label Jul 27, 2023
@vallsv
Copy link
Author

vallsv commented Jul 31, 2023

Would it be satisfactory to you if I added a validate prop to SelectToZoom so you could override the default validation logic with your own?

Yes and no. Because I don't understand why we couldn't have a default behaviour which work. This was the approve of this issue.

But again, what hasMinSize is supposed to achieve? I feel like this stuff is just there to debounce the start of the interaction.

If hasMinSize is effectively used this way, i think there is no reason to tune that behaviour.

  • Because it doesn't provide any feature of the integrator. Is it?
  • Because there is no reason to have different user interaction feeling at different places
  • A very small width or height is a good heuristic, that's probably the way it is done in silx. But you could use something else in the future (that's implementation detail), i don't really want to deal with that.

If hasMinSize is not used this way, i would say any user interaction which is automatically cancelled have to be done really carefully

  • An interaction have to work 99.99%, else it creates frustration (if your mouse doesn't work properly, you pick a new one)
  • I can understand it for the debounce (as i said previously) or when the result is inconsistent because the min/max became equal, for example for a selection of 0 pixel area, but that's not an user interaction problem)

Why is this a problem? I'm arguing that it's a good thing as it avoids mistakes.

I can argue why you think it's a mistake?
If somebody have released a mouse and the resulting selection is a rect of 2000x2 pixels, i am pretty sure it's not a mistake.
To me it's a false positive of the actual heuristic.

Here i have only talked about the general use case.
But i can talk about the specific use case at the ESRF, if you prefer. Because i have already noticed such false positive at the beamline, and more generally because it is my job to know how our UI is used.

@axelboc
Copy link
Contributor

axelboc commented Aug 8, 2023

Thanks for taking the time to discuss this further, much appreciated.

I can argue why you think it's a mistake?

With a sensitive mouse, trying to draw a very thin box is risky. Maybe you want a 2000x5px box but the action of releasing the mouse button makes you draw a 2000x2px box instead. And the thinner the slice, the less context you have to make sure you indeed zoomed at the right place. This is what I meant by "Zooming on a tiny slice can be quite disorienting."

Moreover, drawing a very thin selection box takes a bit of time, either because the mouse is too sensitive and users have to move it very carefully, or because the mouse is not sensitive enough and they have to lift it off the desk a few times.

The idea behind the minZoom/hasMinSize heuristic was therefore twofold:

  • to avoid potential frustration from getting lost while trying to zoom in too quickly and having to zoom out and back in;
  • to teach users to be more efficient when zooming by first drawing a slightly bigger box and then using the wheel-based axial zoom feature to get to the zoom level they want (or drawing another selection box, maybe also axial).

Because i have already noticed such false positive at the beamline

No worries, then! Happy to change the default behaviour if it leads to frustration for end users.

That being said, I encourage you to think about ways to make axial-based zoom more discoverable in your UI so users become more efficient at zooming and do not try to zoom on tiny slices in the first place.


Now we need to decide what happens to the minZoom prop of SelectToZoom:

  • Do we keep it and use it as the minimum zoom "distance" threshold?
  • Do we consider it an implementation detail and remove it in favour of a more generic validate prop?

Either way, what should the default minimum distance be?

@vallsv
Copy link
Author

vallsv commented Aug 30, 2023

That being said, I encourage you to think about ways to make axial-based zoom more discoverable in your UI

I don't know what you mean.

It's the default behaviour of my UI, because it's the default behaviour at the ESRF (used by PyMCA, silx, matplotlib).

That's the way scientists interact with data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants