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

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

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

LinqLover
Copy link
Contributor

  • disable formatting in text fields (>= Squeak 6.0)
  • save version dialog: implement setTextSelector (#message:) correctly to let the view know that the save was successful (will remove the orange triangle)
  • save version dialog: fix cancel to actually reset the text (#okToRevertChanges:) (>= Squeak 6.0)
  • feedback dialog: instead of hacking #editText:, update all dialog texts prior to submitting via #acceptChanges. This way, undo/redo is working as normally and the edit indication semantics of the toolbuilder are maintained.

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.

* disable formatting in text fields (>= Squeak 6.0)
* save version dialog: implement setTextSelector (#message:) correctly to let the view know that the save was successful (will remove the orange triangle)
* save version dialog: fix cancel to actually reset the text (#okToRevertChanges:) (>= Squeak 6.0)
* feedback dialog: instead of hacking #editText:, update all dialog texts prior to submitting via #acceptChanges. This way, undo/redo is working as normally and the edit indication semantics of the toolbuilder are maintained.
@LinqLover LinqLover added the ui label Jun 17, 2022
@LinqLover LinqLover requested a review from j4yk June 17, 2022 18:09
Copy link
Collaborator

@j4yk j4yk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish we could avoid the use of Object>>in: but here it would require to pull the respective widget spec to the top of the method, so the widgets will in the code not anymore be in the order in which they are added as children...

Anyway, please ensure that when cancelling the Save dialog, the message is still retained in the SquotSaveVersionDialog, as commented. Otherwise ok.

@LinqLover LinqLover requested a review from j4yk September 8, 2022 15:15
@j4yk
Copy link
Collaborator

j4yk commented Sep 11, 2022

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.

…l and re-opening the version saver"

This reverts commit 45eedac.

Turned out that there is little sense in restoring the placeholder text permanently. See: hpi-swa#380 (comment)
@LinqLover
Copy link
Contributor Author

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
Copy link
Collaborator

j4yk commented Sep 11, 2022

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
Copy link
Contributor Author

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
Copy link
Collaborator

j4yk commented Sep 14, 2022

Your analysis is correct as far as I am aware.

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

Successfully merging this pull request may close these issues.

2 participants