4ian / GDevelop

🎮 Open-source, cross-platform 2D/3D/multiplayer game engine designed for everyone.
https://gdevelop.io
Other
10.83k stars 850 forks source link

Clicking outside the extension installer pop-up window will initiate an update (and overwrite existing version) #3554

Closed tristanbob closed 2 years ago

tristanbob commented 2 years ago

Describe the bug

Clicking outside the extension installer pop-up window will initiate an update (and overwrite existing version).

To Reproduce

1) Open Project Manager menu 2) Select an extension 3) Click outside the pop-up window (such as accidentally missing the "Open in editor" button by a few pixels) 4) The extension is reinstalled from the extension from the repository, overwriting any changes (and losing work)

Two problems: 1) It should require confirmation before performing the update (new installs don't need confirmation) 2) Clicking outside the pop-up window should act like "Cancel" and go back to Project Manager.

Other details

Video

https://user-images.githubusercontent.com/8879811/151196919-42724fd1-50dc-4d4e-96f5-b883a83453a7.mp4

4ian commented 2 years ago

Can you confirm you have the "click outside" behavior set to "Apply changes" in the preferences?

@ClementPasteau We should remove the onApply from this dialog, it's the second time we're getting the feedback.

ClementPasteau commented 2 years ago

I've just tested it and I can confirm this happens because the preferences are set as "Apply changes when clicking outside of Dialog"

I don't think removing onApply is a good idea, because it would then remove the ability to use the shortcut to submit the dialog (CMD+Enter) I would rather suggest removing the ability to click outside the dialog?

(it's one or the other, not sure what's best)

4ian commented 2 years ago

I don't think removing onApply is a good idea, because it would then remove the ability to use the shortcut to submit the dialog (CMD+Enter)

True, but I wonder if this dialog is not special and we should not consider that it could be 'Ctrl+Enter'ed? Otherwise dismissing the click outside is another solution but not sure what's best yeah 😅

tristanbob commented 2 years ago

I can't think of any reason on this particular pop-up where using "control+enter" makes sense. The "control+enter" keypress only makes sense when your cursor focus is inside a field (i.e. you just typed something) and you want to save it. In this case, this pop-up has NO fields to edit so that use-case can never happen. Therefore, we don't need the OnApply logic. Am I understanding this correctly?

Secondly, can we add a confirmation prompt to the extension upgrade process, specifically when an extension is already installed?

4ian commented 2 years ago

I think both things make sense yeah, no "apply" here (i.e: no ctrl+enter) and probably confirm if we know it's an upgrade.

ClementPasteau commented 2 years ago

I've addressed both points in #3556

tristanbob commented 2 years ago

Thank you so much for fixing this... it means even more now because I just lost more work due to a single missed click. :(

I'm going to see if the nightly build has been published, I don't want this to happen ever again.

4ian commented 2 years ago

Sorry about that @tristanbob :/ We'll have to be more careful about what "apply" means in future dialogs.

tristanbob commented 2 years ago

No need to be sorry, I really like the software you are making.

It's actually kinda ironic because the "save when clicking outside pop up" logic was added specifically to prevent lost work. :)