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

Pen events blocking improvements #672

Draft
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

cyclane
Copy link

@cyclane cyclane commented May 13, 2023

Fixes #665 , possibly #534 :
PenHolder stores pen device on first event and only handles events from that device until the pen is reinstalled.

Pen device source type is stored on first event and events from other device source types are blocked and inhibited from propagating until the pen is up again.

#624 : (not fixed)
The zooming gesture overrides touch drawing and undoes the previous stroke if pen was not idle when the gesture began (and also from the same device source type, otherwise the stroke is ended).

Why both implementations in the same PR?
Both implementations depend on comparing gdk::Devices and since I think that is the main concern for both I just did everything in one PR.

To consider before merging :

  • Currently devices are compared with the gdk::Device::eq (ie. ==) which compares pointers under the hood. It is not clear to me whether this is robust enough or not, but I have not encountered any issues with this in my testing so far.
    • If after-all this is not an option, I believe the only other unique gdk::Device property is gdk::DeviceExt.name() which could be compared instead.
    • Solution: Compare device source types instead of devices.
  • Currently as soon as the zooming gesture ends there might still be one finger on the screen which can result in an unintended stroke if not blocked. Unfortunately a new TouchBegin event is sent for this finger (even though it is already on the screen), so blocking all touch events until the next TouchBegin event is not an option.
    • In my opinion this would probably be quite irritating, but I can't think of a better way to handle this other than blocking touch events for a short period after the gesture.
    • However, in my testing (and I think this should be the general case since <10Hz touchscreens are very unlikely to be used for writing), a short delay of 100ms fixes this issue quite well (commit 3ff0663).
    • Solution: Implementation in the point above is sufficient.
  • I agree with #624 comment in that this shouldn't be an option and should just be implemented since the alternative behaviour is extremely unlikely to be the intended behaviour by the user, and additionally the inputs are coming from the same device so it is difficult to tell the difference between two fingers.
    • Also note the "Block Pinch to zoom" option pretty much does this anyway (as the comment points out).
  • Just undoing the current stroke on zoom isn't great. Ideally the removed stroke should not be in the stroke history at all. Same applies for erasing a stroke.

Note: I have been (and am currently) using these for notes to find any bugs with my implementations. So far I have applied fixes to any problems I have encountered, and more importantly I have not encountered any issues related to how gdk::Devices are compared which I think is the primary concern for this PR.

@cyclane cyclane changed the title Pen events blocking improvements Pen events blocking improvements and zooming with touch drawing May 14, 2023
@flxzt
Copy link
Owner

flxzt commented May 16, 2023

Thanks for doing this work!

A few points:

  1. identifying unique input devices: I am a bit sceptical about pointer-comparing here. While it might work in most cases, I believe it might just be enough to check the same-ness of the input-source ? I also don't see another way to uniquely identify devices, Gtk's devs also told me that wayland simply does not have this capability. Checking VID/HID wouldn't work well either, as some input devices are connected through the same usb device (Logitech unify receivers) or not through usb at all (touchpads through smbus).
  2. This seems "frontend" specific, and I am not sure if we should pass gdk::Device's into the engine at all. I am planning to remove the gtk dependency to the engine entirely. Maybe we can make this work only in rnote-ui ?
  3. Regarding blocking input after a pinch-zoom has finished. I don't see a better way to do this either, so I think it is totally fine

@cyclane
Copy link
Author

cyclane commented May 16, 2023

Thank you very much for your comments!

  1. identifying unique input devices: I am a bit sceptical about pointer-comparing here. While it might work in most cases, I believe it might just be enough to check the same-ness of the input-source ? I also don't see another way to uniquely identify devices, Gtk's devs also told me that wayland simply does not have this capability. Checking VID/HID wouldn't work well either, as some input devices are connected through the same usb device (Logitech unify receivers) or not through usb at all (touchpads through smbus).

Alright. Though I have had no issues, there definitely could be issues under other conditions (or in future versions of GTK e.t.c.) and for most use-cases I think it would be enough to compare input-sources instead since most setups will only have at most one of each type of input if it worked as expected. Unfortunately, on Wayland pens (I have tested both touchscreen stylus and Wacom drawing tablet) are InputSource::Mouse, so issue #665 and #534 would remain unresolved.

  • However, gdk::Device::name is also available and does actually work as expected, so I would suggest this instead of input-source. I think it is possible for the name to change, but since these comparisons are very short-term I do not believe that would cause problems (though this is also why I am not too sceptical about pointer comparisons).
  1. This seems "frontend" specific, and I am not sure if we should pass gdk::Device's into the engine at all. I am planning to remove the gtk dependency to the engine entirely. Maybe we can make this work only in rnote-ui ?

That makes sense. I wasn't aware of this plan so I just implemented it in the easiest way (to at least show that it works). I have pushed an implementation that works almost only in rnote-ui (it needs to be able to know whether engine.pen_progress is PenProgress::Idle so I added RnoteEngine.pen_idle()).

  1. Regarding blocking input after a pinch-zoom has finished. I don't see a better way to do this either, so I think it is totally fine

Okay, I'll keep this implementation then (with 100ms - I think that's a sensible delay to use, not too long but should cover the vast majority of devices).

@cyclane cyclane force-pushed the pen-events-blocking-improvements branch from 6448e34 to c9b51ac Compare May 17, 2023 09:09
@flxzt
Copy link
Owner

flxzt commented May 18, 2023

Alright. Though I have had no issues, there definitely could be issues under other conditions (or in future versions of GTK e.t.c.) and for most use-cases I think it would be enough to compare input-sources instead since most setups will only have at most one of each type of input if it worked as expected. Unfortunately, on Wayland pens (I have tested both touchscreen stylus and Wacom drawing tablet) are InputSource::Mouse, so issue #665 and #534 would remain unresolved.

That's the reason I don't want to rely on it - we would rely on the internal structure of how gtk handles device instances, which could break at any moment.
Regarding using input source, I now remember that I even reported this myself in gtk's bug tracker. Not a good approach then.
But using the name is also not great - on X11 this is always "Virtual Core Pointer" (for stylus, touchpad, touchscreen and mouse). On Wayland this works better, at least the stylus has a different name.
In any case, we should be careful and lean towards a solution that might end up not detecting different devices in some cases.

That makes sense. I wasn't aware of this plan so I just implemented it in the easiest way (to at least show that it works). I have pushed an implementation that works almost only in rnote-ui (it needs to be able to know whether engine.pen_progress is PenProgress::Idle

I think we can simply use current_pen_progress() directly, it already is pub.

Edit:
what about implementing our own input source detection? The way it worked previously while using different gestures was also using certain conditions on a higher level to detect if the current input method is either stylus, mouse or touch input.

  • we already have the stylus detection - checking if device_tool exists. I believe this is also what gtk does inside GtkGestureStylus

  • detecting touch input is trivial, all touch events are the condition for that

  • other input can be mouse input - this does not differentiate between phyiscal mouse and touchpad, so that would not allow detecting between them. But to me it seems that the bug is mostly related to switching stylus tools (pen/eraser), using touchscreen and or switching between those input methods and the mouse/touchpad.

@cyclane
Copy link
Author

cyclane commented May 18, 2023

But using the name is also not great - on X11 this is always "Virtual Core Pointer" (for stylus, touchpad, touchscreen and mouse). On Wayland this works better, at least the stylus has a different name.

As far as I am aware on X11 everything uses the same gdk::Device, so #534 and #665 are impossible anyway since any updates from devices will move the used pointer by a minimal/zero amount.
This can be demonstrated by trying to draw with a pen and mouse intentionally (with current Rnote, not this PR):

In any case, we should be careful and lean towards a solution that may end up not detecting different devices in some cases.

I agree - ideally this would be using input-source which we have ruled out. I think using names gives almost the same effect as comparing devices and is fine to do since it is extremely unlikely (if not impossible) for the name to unintentionally change during a stroke. Additionally, it is less likely to break in the future in the way the pointer comparison could.

Edit:
what about implementing our own input source detection? The way it worked previously while using different gestures was also using certain conditions on a higher level to detect if the current input method is either stylus, mouse or touch input.

That would work as well, but I think there is no point if we can find a good way to compare devices. I think comparing names would be fine, but custom input source detection is safer (in the sense we know what is going on) than comparing names.

I guess the main question is do you agree that comparing names is fine (considering X11 doesn't have the bug to begin with), or would you prefer to go the safer route of custom input source detection? I would stick to the name approach, but am happy to change the implementation to custom input source detection if you think that would be better.

@flxzt
Copy link
Owner

flxzt commented May 18, 2023

On X11 you just make the line squiggly by moving the used pointer relative to its current position.

Right, there can't be a second cursor at a different position, but that wouldn't necessarily mean that the occasional jump by a faulty touchscreen/touchpad couldn't happen, would it? To me it sounds like it still would be an improvement if other input is blocked for the duration of an in-progress stroke. I am not sure I understand your argument.

That would work as well, but I think there is no point if we can find a good way to compare devices. I think comparing names would be fine, but custom input source detection is safer (in the sense we know what is going on) than comparing names.

Yeah but I wouldn't consider comparing the names a good way to detect the different devices. At least on X11 the case definitely exists where the same name would be reported, but our custom detection would recognize a difference in input source.
I think going with our own detection would be more robust and mimic gtk's detection in it's gestures, which has already been proven to work relatively well.

@cyclane
Copy link
Author

cyclane commented May 18, 2023

Right, there can't be a second cursor at a different position, but that wouldn't necessarily mean that the occasional jump by a faulty touchscreen/touchpad couldn't happen, would it? To me it sounds like it still would be an improvement if other input is blocked for the duration of an in-progress stroke. I am not sure I understand your argument.

Since touchpads are relative it would need a very broken touchpad to get the issue I think, but you're right - it doesn't solve a faulty touchscreen.

Yeah but I wouldn't consider comparing the names a good way to detect the different devices. At least on X11 the case definitely exists where the same name would be reported, but our custom detection would recognize a difference in input source.
I think going with our own detection would be more robust and mimic gtk's detection in it's gestures, which has already been proven to work relatively well.

Okay, I see your point now - I didn't realise on X11 touch events still have their touch event types, even though the device is the same (Virtual Core Pointer). I'd like to note though the stylus detection does not work on X11 (due to everything being the same device I'm guessing).

I have pushed an implementation with manual input source detection. I still used the gdk::InputSource enum so that if GTK does fix their input source Rnote can easily switch to that.

@flxzt
Copy link
Owner

flxzt commented May 21, 2023

I still used the gdk::InputSource enum so that if GTK does fix their input source Rnote can easily switch to that.

Sounds good. Maybe a more detailed comment explaining this above input_source_from_event() would be nice for our future selves.

Okay, the input blocking part looks good, I see that you used PenState, which is probably better when thinking about it - using PenProgress could result in a "locked" state where it might be unclear to the user how to finish an in-progress stroke.

Regarding the zooming while having the touch-drawing option enabled - unfortunately this does not work well on X11. Touch events still are drawn as strokes on the canvas when a zoom-pinch starts, and also sometimes when it ends.

Also - undoing is not ideal here, I don't think the user would ever want to have this stroke in the history. We should probably remove those strokes entirely out of the StrokeStore. That would mean we need to account for that in a different part of the StrokeStore here. I think we would need to explicitely move the rendering components from the history into the current engine.

@cyclane
Copy link
Author

cyclane commented May 22, 2023

Regarding the zooming while having the touch-drawing option enabled - unfortunately this does not work well on X11. Touch events still are drawn as strokes on the canvas when a zoom-pinch starts, and also sometimes when it ends.

I forgot to test X11 after changing the implementation to use input sources. It is fixed now.

(issue was that I used input_source_from_event() for getting all input sources other than from gestures, so the gesture input source was always gdk::InputSource::Mouse and so the comparisons for undoing strokes and blocking events after the gesture were failing).

Also - undoing is not ideal here, I don't think the user would ever want to have this stroke in the history. We should probably remove those strokes entirely out of the StrokeStore. That would mean we need to account for that in a different part of the StrokeStore here. I think we would need to explicitely move the rendering components from the history into the current engine.

Yes, I'm not sure why I overlooked that. I don't quite understand why anything needs to be accounted for in StrokeStore.import_history_entry(), but also I don't think it should be handled only for strokes since otherwise other (not Brush) pens could still result in unintended selections / erases e.t.c. . I would suggest maybe adding a cancel_action: bool (or similar) property to PenEvent::Cancel and handling cancelling the current action for each pen?

Edit:
Or simply we can just undo and then remove future history. I've implemented this in 5e13634 as a demonstration since it's quite easy to do. However, not everything gets fixed with this method (e.g. selections will un-select), but brush strokes and the eraser is cancelled and undone properly.

So if that is enough we can just do this since the beginning of the stroke just before a zoom will remove future history anyway, so removing it again after a zoom is detected should only ever remove the history that we don't want to have.

@flxzt
Copy link
Owner

flxzt commented May 26, 2023

Regarding the zooming:

I made some changes that are improving some of the mentioned issues:

  • 510d8a0 should allow removing strokes entirely out of the engine without breaking the rendering
  • 689a49e refactored the way the history is handled - the current state is now recorded into the history after a change has happened. This could enable removing an already drawn stroke by passing something like cancel_action with a PenEvent::Cancel similar to how you described it, but without having to undo & remove future (which in my opinion is a hack and shouldn't be done if we can avoid it)

But I am starting to doubt that this is the best way to implement this. There will always be some unsolvable jankiness to it, like moving the typewriter textbox and deselecting. And I also don't like that the to be deleted strokes are visible for a short time, it looks like a glitch.

Going forward maybe we can approach this differently, and implement some alternative controls when touch-drawing is enabled - like easier to access zoom controls (for example as a separate overlay, like in previous versions) and a better way to drag the view around (maybe like a software joystick or something like that?)

What do you think? I definitely want the input source part of this PR to land, so maybe would be best to split and do another PR/issue/discussion for the zooming, which in my opinion still requires more thought and work

@cyclane
Copy link
Author

cyclane commented May 27, 2023

I made some changes that are improving some of the mentioned issues:

  • 510d8a0 should allow removing strokes entirely out of the engine without breaking the rendering

  • 689a49e refactored the way the history is handled - the current state is now recorded into the history after a change has happened. This could enable removing an already drawn stroke by passing something like cancel_action with a PenEvent::Cancel similar to how you described it, but without having to undo & remove future (which in my opinion is a hack and shouldn't be done if we can avoid it)

Okay thank you. I think this means the zooming gesture with touch drawing can be implemented without any stroke-history issues now.

But I am starting to doubt that this is the best way to implement this. There will always be some unsolvable jankiness to it, like moving the typewriter textbox and deselecting. And I also don't like that the to be deleted strokes are visible for a short time, it looks like a glitch.

Going forward maybe we can approach this differently, and implement some alternative controls when touch-drawing is enabled - like easier to access zoom controls (for example as a separate overlay, like in previous versions) and a better way to drag the view around (maybe like a software joystick or something like that?)

This implementation is from comment in #624 which I thought would be the easiest to use solution. I don't understand why we can't solve the typewriter textbox and deselecting if we handle each brush separately through PenEvent::Cancel (can't we just reset the position / selection state when zooming starts?)

I see your point about the line before the zooming, which we obviously can't remove. It looks a little weird, but doesn't actually affect anything after-all (and doesn't even appear if you manage to get both fingers on the screen at once). I think alternative controls could also work, but I don't see how we could make that as fast & easy to use as the normal zooming gesture which I think is a bigger issue than a temporary stroke.

I definitely want the input source part of this PR to land, so maybe would be best to split and do another PR/issue/discussion for the zooming, which in my opinion still requires more thought and work

Yeah I agree, I've removed the zooming implementations from this PR so it can be merged. I'm thinking let's continue the discussion back in #624 to decide whether we should try this implementation with all the history issues fixed first, or do something different entirely?

rnote-ui/src/canvas/input.rs Outdated Show resolved Hide resolved
rnote-ui/src/canvas/input.rs Outdated Show resolved Hide resolved
@flxzt
Copy link
Owner

flxzt commented May 27, 2023

Okay thank you. I think this means the zooming gesture with touch drawing can be implemented without any stroke-history issues now.

Yes, but it would still need an additional flag in the Cancel events to tell the pens that the current state should not be recorded and any strokes/state that was committed during the progress of the pen should be reverted.

why we can't solve the typewriter textbox and deselecting if we handle each brush separately through PenEvent::Cancel (can't we just reset the position / selection state when zooming starts?)

Yeah we could do that, but it would require more work. For example: as soon as we are in the selecting state of the selector, we don't have the last selected strokes anymore that would be needed to revert the unselect. Another example: We would need to save the trashed strokes in the eraser, or when it is in the "split-stroke" mode save the state of the modified strokes and revert that.

And implementing it in this way still would have the issue of visibly doing & reverting something on the canvas..

But yeah, we can continue this discussion in #624 . I'd love to get zooming/better navigation working in combination with touch-drawing, so let's discuss more possible solutions there

@cyclane cyclane requested a review from flxzt May 27, 2023 22:28
@flxzt
Copy link
Owner

flxzt commented May 29, 2023

Let's leave #534 open until it is confirmed fixed.

Testing this is currently difficult because gnome-shell is constantly crashing for me when using a stylus (see this bug ) but here is what I observed on X11:

first input method second input method behaviour
touch stylus the first stroke gets finished, and a second is created for the stylus input. This seems to be because gtk emits a touch-end or touch-cancel event. After the stylus is finished, another stroke is generated when continuing to draw with touch
touch mouse if the mouse is only moved the events seem to be correctly rejected. But if the mouse clicked down and drags, the view gets moved
mouse stylus the strokes jumps to and back from the stylus, without being finished. Shouldn't the stylus input be rejected here?
mouse touch This is a bit problematic: The touch input is rejected sometimes, but this then causes the event to be propagated to the drag-canvas gesture, which then moves the view around. And not every event seems to be rejected, so the stroke still jumps to the touch input
stylus touch after starting a stroke with the stylus, on my device there aren't any touch events coming in. I think this is because palm rejection on my device works well. I don't know if they are correctly rejected for devices where palm rejection at the driver level doesn't work well.
stylus mouse The stroke zips around a bit, but continues. Some events seem to be rejected but not all.

I believe we need to differentiate between "reject but let the event continue to propagate", and "reject and drop the event" to resolve some of these issues.

I also noticed that the pen state sometimes goes into an "invalid" state where a "pen up" seems to be missing so the stroke continues to be drawn even when the mouse button is released. I couldn't figure out how to consistently reproduce this.

So there are still some unresolved issues/even new bugs that get introduced by this. Since this is really critical to the core functionality of the app we need to be careful here

@cyclane
Copy link
Author

cyclane commented May 31, 2023

Thank you for the testing!

touch | stylus | the first stroke gets finished, and a second is created for the stylus input. This seems to be because gtk emits a touch-end or touch-cancel event. After the stylus is finished, another stroke is generated when continuing to draw with touch

Unfortunately I cannot reproduce this and so can't really test it...

So it seems maybe it's device-specific, and not gtk's fault? If this is the case I'm not sure how this could be sensibly fixed. The only 'solution' I can think of is a short delay after touch-end and touch-cancel that rejects any new strokes, but I think it would be better to leave this issue that doing something like that.

touch | mouse | if the mouse is only moved the events seem to be correctly rejected. But if the mouse clicked down and drags, The view gets moved

I think I have fixed this with your suggestion of inhibiting events from other devices instead of just blocking them.

mouse | stylus | the strokes jumps to and back from the stylus, without being finished. Shouldn't the stylus input be rejected here?

In a previous message I mentioned that stylus detection does not work on X11 - everything is the same device (Virtual Core Pointer) and so no device tool exists, even if it is a stylus.

I think this is the cause of this issue?

This works properly (with the stylus input being rejected) on Wayland.

mouse | touch | This seems a bit problematic: The touch input seems to be rejected sometimes, but this then causes the event to be propagated to the drag-canvas gesture, which then moves the view around. And not every event seems to be rejected, so the stroke still jumps to the touch input

Testing on my device it seems when the mouse is down touchscreen events come in as mouse events, seems to be an X11 issue (I guess probably another extension of the fact everything uses the same pointer?).

stylus | touch | after starting a stroke with the stylus, there don't seem to be any touch events coming in. I think this is because palm rejection on my device works well. I don't know if they are correctly rejected for devices where palm rejection at the driver level doesn't work well.

I believe we can simulate this by just using a finger (in my case I receive touchscreen events and they are rejected as intended).

stylus | mouse | The stroke zips around a bit, but continues. Some events seem to be rejected but not all.

Similar to mouse | stylus, I believe this is due to the stylus appearing as a mouse on X11 (due to the lack of a .device_tool()). Again this works fine in Wayland for me.


In summary:

  • Some issues were fixed by inhibiting events rejected due to the device source type check.
  • Most of the of the other issues are X11-specific, all related to the source type of the event being reported / detected incorrectly. We could implement rejection based on the velocity change of the inputs to solve some of these issue I think, but still not all (for example stylus | mouse would not be solved since the mouse is relative and so won't cause any 'unusual' velocity changes (and also this would be much more complicated than the current implementation).

I believe we need to differentiate between "reject but let the event continue to propagate", and "reject and drop the event" to resolve some of these issues.

Yes it solved the touch | mouse issue, and also the other combinations on Wayland (which would drag instead of having the issues you described).

I also noticed that the pen state sometimes goes into an "invalid" state where a "pen up" seems to be missing so the stroke continues to be drawn even when the mouse button is released. I couldn't figure out how to consistently reproduce this.

I haven't experienced this at all... I'll try to do some X11 testing, maybe it's X11-specific again. Though I don't quite see how such an issue could be caused by this implementation specifically...

Also I have left a comment in #624 to continue the discussion on zooming with touch-drawing.

@cyclane cyclane changed the title Pen events blocking improvements and zooming with touch drawing Pen events blocking improvements May 31, 2023
@cyclane cyclane force-pushed the pen-events-blocking-improvements branch from 6759a26 to 396657b Compare May 31, 2023 22:25
@flxzt flxzt added this to the v0.7 milestone Jun 6, 2023
@flxzt
Copy link
Owner

flxzt commented Jun 12, 2023

Alright, I tested it again with the change you made:

on Wayland:

first input method second input method behaviour
mouse touch the stroke stops. Then, when lifting touch the following error message gets logged: `gdk_button_event_get_button: assertion 'GDK_IS_EVENT_TYPE (event, GDK_BUTTON_PRESS)
touch mouse input is ignored correctly, but after lifting mouse same error as above
stylus touch is ignored correctly
touch stylus stroke stops and a second one gets drawn with stylus
mouse stylus the stroke zip zaps between both input methods, even when stylus is only hovering
stylus mouse stroke zips between them, even when mouse click is released and only moved

on X11:

first input method second input method behaviour
mouse touch stroke zips to touch input, sometimes the following Error is logged: `gdk_button_event_get_button: assertion 'GDK_IS_EVENT_TYPE (event, GDK_BUTTON_PRESS)
touch mouse is ignored correctly
stylus touch enters a state where the stroke continues, but after lifting the touch and stylus, no stylus & touch input can be made anymore, it draws while mouse is up, only a mouse click and release stops the stroke
touch stylus new stroke when starting to draw with stylus, but then sometimes has the same behaviour as touch -> stylus
mouse stylus stroke continues and jumps to the stylus
stylus mouse zips around the stylus input a little bit, then after lifting stylus and releasing mouse click enters a state where stroke continues to be drawn while only moving the mouse. Only another click -> release finishes it.

I believe the problem of moving the view is fixed now, but there are still some issues with going into "invalid" pen states because most likely some events from other sources are not recognized while others are rejected. So the event stream is not consistent anymore because button pressed -> release events are not always paired together.

We would probably need to log the events for each case here to understand why certain events are not properly recognized and rejected. From my past experience gtk's event data is very inconsistent between different event types, for example move events might report the correct source, while button events don't have that information. I am not sure how we could fix/work around this on the app level.

Addressing your comment:

In a previous message I mentioned that stylus detection does not work on X11 - everything is the same device (Virtual Core Pointer) and so no device tool exists, even if it is a stylus.

I do think we are able to differentiate - the device tool should exist for the stylus, because thats also how a pressed stylus eraser button is recognized - and that works on X11.

Most of the of the other issues are X11-specific, all related to the source type of the event being reported / detected incorrectly. We could implement rejection based on the velocity change of the inputs to solve some of these issue I think, but still not all (for example stylus | mouse would not be solved since the mouse is relative and so won't cause any 'unusual' velocity changes (and also this would be much more complicated than the current implementation).

Yeah detecting the correct source on X11 seems to be pretty difficult/impossible in the app. Adding rejection based on velocity could be another possible solution, but I can only speculate how robust and error-prone it would be.

@cyclane
Copy link
Author

cyclane commented Jun 13, 2023

Thank you for testing again, I don't have much time at the moment, so I'll take a deeper look into all of this at the end of next week, but for now here are my thoughts:

  • Testing on KDE Plasma 5.27 Wayland I cannot reproduce any of the Wayland issues you mention, BUT I can reproduce them on GNOME 44. So it seems to me the behaviour can also vary between compositors.
  • I have managed to reproduce most/all of the X11 issues in GNOME (KDE X11 is crashing for some reason, so I couldn't test) and most of them seem to be from the stylus not having a device tool.

Once I get around to looking into this deeper next week I'll make a list of the cause of each issue and implement solutions if I come up with any. Seeing how wildly this varies between different environments, I will also test this on Windows to see what sort of issues come up there.

I do think we are able to differentiate - the device tool should exist for the stylus, because thats also how a pressed stylus eraser button is recognized - and that works on X11.

I don't get a device tool on X11... If you do maybe it's yet another device-specific thing.

Yeah detecting the correct source on X11 seems to be pretty difficult/impossible in the app. Adding rejection based on velocity could be another possible solution, but I can only speculate how robust and error-prone it would be.

Yeah, The velocity thing wouldn't even work if the two devices are pointing close to each other, so I don't think it's worth considering at all. I think X11 will probably have to remain with some issues in the end.

@flxzt flxzt removed this from the v0.7 milestone Jun 16, 2023
@cyclane
Copy link
Author

cyclane commented Jul 30, 2023

Apologies for the much longer than anticipated delay.

I've looked at X11/Wayland on GNOME/KDE, here's what I have gathered. Note all logs are collected at the beginning of handle_pointer_controller_event like so:

pub(crate) fn handle_pointer_controller_event(
    canvas: &RnCanvas,
    event: &gdk::Event,
    mut state: PenState,
) -> (Inhibit, PenState) {
    let now = Instant::now();
    let mut widget_flags = WidgetFlags::default();
    let touch_drawing = canvas.touch_drawing();
    let gdk_event_type = event.event_type();
    let gdk_modifiers = event.modifier_state();
    let _gdk_device = event.device().unwrap();
    let backlog_policy = canvas.engine_ref().penholder.backlog_policy;
    let input_source = input_source_from_event(event);
    let is_stylus = input_source == Some(gdk::InputSource::Pen);

    //std::thread::sleep(std::time::Duration::from_millis(100));
    //super::input::debug_gdk_event(event);

    let device_tool = event.device_tool();
    let pen_input_source = canvas.pen_input_source();

    let inhibit_o = reject_pointer_input(
        event,
        state,
        input_source == canvas.pen_input_source(),
        touch_drawing,
    );
    log::debug!("event={gdk_event_type:?} input_source={input_source:?} inhibit={inhibit_o:?} device={_gdk_device:?} device_tool={device_tool:?} | state={state:?} pen_input_source={pen_input_source:?}");

    if let Some(inhibit) = inhibit_o {
        return (inhibit, state);
    }
    ...

X11

KDE Plasma (kwin) 5.27.6

  • mouse then touch (touch drawing enabled)
    • Touch input is inhibited correctly, but pointer still moves (because everything uses the same pointer in X11).
    • This causes the stroke to go the the touch position once the next mouse input comes in.
  • mouse then touch (touch drawing disabled)
    • For some reason even if pointer_controller has Capture as its propagation phase, and canvas_zoom_gesture has Bubble or even Target, the zoom even is still received first and so is not inhibitted as expected.
  • touch then mouse (touch drawing enabled)
    • Similar issue to mouse then touch (touch drawing enabled), except the pointer moves relatively so much less noticeable.
  • touch then mouse (touch drawing disabled)
    • Drawing starts at finger as expected
  • stylus then touch (touch drawing enabled)
    • input ignored correctly (not the same as mouse then touch because the stylus has an absolute position, unlike the mouse)
  • stylus then touch (touch drawing disabled)
    • Similar issue to mouse then touch (touch drawing disabled).
  • touch then stylus (touch drawing enabled)
    • input ignored correctly
  • touch then stylus (touch drawing disabled)
    • Similar issue to mouse then touch (touch drawing disabled).
  • mouse then stylus
    • Similar issue to mouse then touch (touch drawing enabled).
  • stylus then mouse
    • Similar issue to touch then mouse (touch drawing enabled).

GNOME (mutter) 44

Exactly the same as X11 KDE (kwin) 5.27.6 .

Wayland

KDE Plasma (kwin) 5.27.6

  • mouse then touch
    • input ignored correctly
  • touch then mouse
    • input ignored correctly
  • stylus then touch
    • input ignored correctly
  • touch then stylus
    • input ignored correctly
  • mouse then stylus
    • input ignored correctly
  • stylus then mouse
    • input ignored correctly

GNOME (mutter)

  • mouse then touch
    • On touch all mouse events stop being sent, pausing the input while there is touch input.
    • If the mouse is let go, the ButtonRelease event is never received, meaning once touch is also let go Rnote thinks the pen is still down even though it is not.
    • This seems to be a GTK issue so I'm not sure we can fix this (considering the events are not being sent at all).
  • touch then mouse
    • input ignored correctly
  • stylus then touch
    • input ignored correctly
  • touch then stylus
    • input ignored correctly
  • mouse then stylus
    • input ignored correctly
  • stylus then mouse
    • input ignored correctly

Note: I was unable to replicate the behaviour you found with GNOME Wayland...

Summary

Wayland on KDE works perfectly, and on GNOME mostly perfectly apart from the mouse then touch, which I think is a GTK issue that can't be fixed in Rnote.

However, you reported many more issues on GNOME Wayland, so it seems to be very device-specific and I can't really look into that further myself.

As for X11, most of the issues arise from the nature of X11 having the same pointer for everything, and so cannot be fixed. mouse then touch (touch drawing disabled) could be improved, but then it'll just have the same issue as mouse then touch (touch drawing enabled).

I think I've done all I can with my device, so for the issues you had with GNOME Wayland it'd be great if you could investigate them on your device.

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

Successfully merging this pull request may close these issues.

Pointer events from other devices cause pen to jump
2 participants