hpi-swa / Squot

Squeak Object Tracker - Version control for arbitrary objects, currently with Git storage
Other
58 stars 29 forks source link

ui: fixes for dialog text fields (save version dialog + feedback dialog) #380

Open LinqLover opened 2 years ago

LinqLover commented 2 years ago

Testing plan

Seems to work for my usual interaction patterns with these dialogs. An additional pair of hands would not harm, though. :-) I did not test this in <Squeak 6.0 so far.

j4yk commented 1 year ago

I am a bit afraid that some will complain that it is unintuitive that Cmd+l does not revert back to the text that you previously accepted with Cmd+s. At least you can get it back with Undo in Squeak 6.0, but not in previous releases.

LinqLover commented 1 year ago

I am a bit afraid that some will complain that it is unintuitive that Cmd+l does not revert back to the text that you previously accepted with Cmd+s. At least you can get it back with Undo in Squeak 6.0, but not in previous releases.

I see you ... How important it is for you to maintain the exact cancel semantics from previous Squeak releases? One might even argue that the former behavior was incomplete/buggy, so I really don't know how much emphasis we should put on this type of compatibility here ... Would you like to me to sprinkle the toolbuilder code with version switches or rather leave it as-is?

j4yk commented 1 year ago

How important it is for you to maintain the exact cancel semantics from previous Squeak releases?

Have they changed? I was not thinking about this special case of the save dialog, but about how text fields in Squeak behave in general. If you "cancel" one text field, you get back to what you "accepted" in it most recently, like in a Workspace. Now it does not add any value to transfer these semantics across different invocations of the save dialog (using the cancel button, and repeating the text from the previous commit does not make sense either), but while you are dealing with exactly one instance of the save dialog, some might insist on that text field behavior. On the other hand there is Tom's pull request to actually finish the commit when you press Cmd+s, which would not fit this expectation either... #268 The dialog is somehow in between a modal dialog and a real tool window.

As far as I am personally concerned: it would not matter to me, since I do not remember ever having cancelling the text field for the commit message with Cmd+l... except for today when testing this PR.

One might even argue that the former behavior was incomplete/buggy, so I really don't know how much emphasis we should put on this type of compatibility here ...

Then please enlighten me what exactly you mean with the "former" behavior. Because apart from introducing the capability to undo a "cancel" I do not remember any general changes to the way it works in Squeak.

Would you like to me to sprinkle the toolbuilder code with version switches or rather leave it as-is?

If you put it that way, no, don't sprinkle it with switches, but please make me get the full picture first.

LinqLover commented 1 year ago

Sorry, you were right and I was wrong, the semantics didn't change (or maybe only temporarily). It's only new that the history is preserved after cancels. Details are in Morphic-mt.1854 and Morphic-mt.1852.

So, with this in mind, let's readdress the cancel semantics in the save version dialog:

Squeak6.0 Squeak 5.3
Before
  • cancel does nothing (just removes changes indication)
  • Cmd + Z does nothing but readds the changes indication
  • cancel does nothing (just removes changes indication and clears change history)
  • Cmd + Z does nothing
After
  • cancel restores the default commit message (but the previous draft will be remembered beyond the dialog's lifetime unless the user types in a new draft)
  • Cmd + Z restores the previous drafted message
  • cancel does nothing (just removes changes indication and clears change history)
  • Cmd + Z does nothing

So at least there was no regression, right?

And yes, I also use this feature extremely rarely. But the Squeak 5.3 behavior looked weird to me. I would also be happy with actually remembering two states in the dialog (last accepted + currently typed) so that cancel could revert to the first one in Squeak 5.3 and Squeak 6.0, but given #268 this solution would not endure a long time anyway. :D

j4yk commented 1 year ago

Your analysis is correct as far as I am aware.