-
-
Notifications
You must be signed in to change notification settings - Fork 298
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for doing this work! A few points:
|
Thank you very much for your comments!
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
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
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). |
6448e34
to
c9b51ac
Compare
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.
I think we can simply use Edit:
|
As far as I am aware on X11 everything uses the same
I agree - ideally this would be using
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. |
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.
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. |
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.
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. |
Sounds good. Maybe a more detailed comment explaining this above Okay, the input blocking part looks good, I see that you used 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 |
I forgot to test X11 after changing the implementation to use input sources. It is fixed now. (issue was that I used
Yes, I'm not sure why I overlooked that. I don't quite understand why anything needs to be accounted for in Edit: 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. |
Regarding the zooming: I made some changes that are improving some of the mentioned issues:
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 |
Okay thank you. I think this means the zooming gesture with touch drawing can be implemented without any stroke-history issues now.
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 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.
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? |
Yes, but it would still need an additional flag in the
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 |
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:
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 |
Thank you for the testing!
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
I think I have fixed this with your suggestion of inhibiting events from other devices instead of just blocking them.
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.
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?).
I believe we can simulate this by just using a finger (in my case I receive touchscreen events and they are rejected as intended).
Similar to In summary:
Yes it solved the
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. |
6759a26
to
396657b
Compare
Alright, I tested it again with the change you made: on Wayland:
on X11:
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:
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.
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. |
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:
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 don't get a device tool on X11... If you do maybe it's yet another device-specific thing.
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. |
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 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);
}
... X11KDE Plasma (kwin) 5.27.6
GNOME (mutter) 44Exactly the same as X11 KDE (kwin) 5.27.6 . WaylandKDE Plasma (kwin) 5.27.6
GNOME (mutter)
Note: I was unable to replicate the behaviour you found with GNOME Wayland... SummaryWayland 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. |
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 :
==
) 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.TouchBegin
event is sent for this finger (even though it is already on the screen), so blocking all touch events until the nextTouchBegin
event is not an option.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.