Interrupting the temporary Eraser tool causes white to be painted instead

[HeCorr]

Issue Summary

When using the Eraser tool via it's temporary shortcut (Ctrl + Shift) and a pop-up window appears (i.e. auto-save or save reminder), the tool is interrupted and the region that was erased turns white as if it was painted.

The same behavior can be observed when Alt-Tabbing.

This does not happen with the actual Eraser tool (shortcut E).

Actual Results

Erased area turns white.

Expected Results

Erased area would remain erased.

Video or Image Reference

2023-08-12.18-28-23.mp4

Steps to reproduce

1. Launch Pencil2D
2. Enable Auto-Save in the preferences and set number of modifications to 4
3. Create a bitmap layer if you don't have one already
4. Paint a large region in dark grey
5. Create a new bitmap layer above the current one and select it
6. Paint a smaller region in black inside the grey region
7. Activate the temporary Eraser tool by holding Ctrl + Shift
8. Erase parts of the black region until the save reminder window appears (may need to release and start a new stroke)
9. Click "No"

System Information

• Pencil2D Version:

Version: 0.6.6
commit: ac415788dfb21f66e1cbcc1ad60443225a2d0e37
date: 2021-02-17_13:03:37
Qt Version: 5.12.10

(Though I believe the issue is still present in the master branch since I've been using a modified version)

• Operating System:
  Windows 10 64-bit

• RAM Size:
  32GB

• Graphics Tablet:
  (irrelevant to this issue, but) Wacom CTL-470

[HeCorr]

I have a potential solution in mind. Can I please be assigned to this? :)

[J5lx]

Sure :)

[HeCorr]

So, the best way I can think of to fix this is to prevent temporary tools from being disabled on focus loss (by deleting this code), and somehow make Pencil detect whether the keys are no longer being pressed after the focus is regained (otherwise the temporary tool is stuck in the active state until you press and release it's keys once).

Would this be an acceptable fix?

Please note that the temporary tool will not get stuck active if the popup window closes while still holding it's shortcut keys and then release them. It will only happen if the user releases the shortcuts while the popup is active/the main window is out of focus, so maybe that is an acceptable trade-off to fix the Eraser issue.

I can explain my understanding of what's causing the issue soon so you guys can potentially come up with a better fix.

[J5lx]

Sorry for the late reply, I forgot to come back to your message. Your proposed fix does sound acceptable to me, though its viability hinges on whether it’s possible to reliably determine the key state. If Qt maintains the key state itself based on keyup/down events from the OS, you’re likely out of luck. From the top of my head I don’t have any other ideas either right now, but maybe @scribblemaniac has some since he did a bunch of work on the temporary tool handling.

Edit: Looks like determining the key state could indeed be a problem. From QGuiApplication::keyboardModifiers() documentation:

It should be noted this may not reflect the actual keys held on the input device at the time of calling but rather the modifiers as last reported in [the QEvent::KeyPress and QEvent::KeyRelease] events.

[HeCorr]

Yeah I figured it would be only event-based.

Maybe we can somehow keep track of key states while the popup is visible, so when it's closed the main window will know whether the temporary keys got released or not if that makes sense.

[J5lx]

I just had another idea… maybe you could try changing the connection between Editor::needSave and MainWindow2::autoSave from the default Qt::AutoConnection to Qt::QueuedConnection so it runs asynchronously.

[scribblemaniac]

Keeping track of the key states is not going to be a feasible solution. Qt simply doesn't provide that information, and there are situations, such as alt-tabbing on Windows which will result in the key release event never being called.

I haven't given this issue a whole lot of thought, but I did have a simple idea yesterday. What you could do is split up the functionality of the Editor::backup function into one or more functions to create backup elements, and one function to append a backup element to the history. And the current backup function could be modified to use these functions so that all the calls to backup do not need to be changed. This solves your issue by being able to create a backup element, apply the stroke and then add it to the backup history (which potentially triggers the autosave).

In editor:

// Backup creation functions
public BackupBitmapElement* createBackupBitmap() { ... }
public BackupVectorElement* createBackupVector() { ... }
...
// Or
public BackupElement* createBackup() { ... }
// Add backup function
public bool addBackup(BackupElement*) { ... }

In EraserTool::pointerReleaseEvent:

{
    BackupElement* backup = mEditor()->createBackup();
    drawStroke();
    mEditor()->addBackup(backup);

Having said all that, @J5lx's idea is good too. In fact, I prefer it over my solution, if only because it would not conflict with #1644.

[HeCorr]

Thanks for the suggestions! I'll be trying Jakob's idea soon and if that doesn't work I'll try scribblemaniac's.

[HeCorr]

Sorry for not working on this yet, I've been busy with other projects.

[J5lx]

Don’t worry, we’ve all been there. Thanks for keeping us posted!

[Jose-Moreno]

@HeCorr I agree with J5lx. This project is a labour of love, and we aren't requiring you to fulfill any quota.

It is already appreciated that you're donating your time and contributing to the software, so take your time and hopefully you can return to this in due time 😃