search

hpi-swa / Squot

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

Adapt the SaveVersionDialog text field to use ctr+s for accept #268

Open tom95 opened 4 years ago

tom95 commented 4 years ago

While this change works, I had to drop the method updateMessageFromViews as it would have send the accept message to the text field on cancel as well. Sadly, I did not understand what the idea behind updateMessageFromViews was, @j4yk could you maybe explain? I'm a bit afraid this removes functionality I was not aware of.

tom95 commented 4 years ago

Oh, how could that change in the postscript have come to be? I created this commit using the latest develop version of Squot and the commit dialog did not show changes to the postscript being staged.

j4yk commented 4 years ago

updateMessageFromViews is there so you need not accept the text in the box first to update it in the model. It means you can type the text, not press ctrl+s, and then click commit and the typed text will be accepted.

j4yk commented 4 years ago

@LinqLover also has run into a postscript issue in #258 and I have as well, as can be seen here: 9254dfd, cb398d1

But I did not find the cause yet, at least if Monticello-ct.715 is not the only cause. Please open a separate issue and check the following: does the package currently have a postscript in your image (before the commit, after the commit)?

j4yk commented 4 years ago

Now on to the actual pull request: do you want to finish the commit by pressing ctrl+s in the text field, or is it something different? Because if you wanted to "double confirm" the message (first ctrl+s, then press commit) instead, I would not like this.

tom95 commented 4 years ago

Hey Jakob, thank you for that detailed list of requirements, that's exactly what I was looking for :) I already kinda suspected that it was meant to persist the text across invocations but didn't manage to follow the control/data flow to make that happen.

The intent was to have a keyboard-based way to finish a commit, so the behavior in my image right now is that I can either press the "Accept" button or hit ctrl+s to confirm and perform the transaction. This is consistent with all other dialog windows in Squeak where accept also means confirm. I suppose in this way you can also look at editing a method as a transaction.

I will update the PR in a bit to address all issues. Sorry that it looks a bit messy, I only looked at the GitBrowser's diff before placing the PR and didn't notice the extra changes.

does the package currently have a postscript in your image (before the commit, after the commit)?

Could you tell me how to best find the postscript in my image? Would I inspect the artifacts in my the GitBrowser at the respective commits? The postload for Squot.core contains the script in both this PR's commit and its parent.

j4yk commented 4 years ago

"Loaded" scripts you find by right-clicking on the package in the object list in the Git Browser (bottom right). Or in the Monticello browser (working copy browser) select the package and use the Scripts button.

Historical scripts you can find by selecting the commit, in the objects list choose "Explore" in the package in question, select the #artifact item in the explorer and inspect self shadowOfTrackedObject (it will trigger the lazy loading of the files). You should get an inspector on a SquotPackageShadow, where you can inspect the MCSnapshot and see whether it contains an MCPostscriptDefinition.

j4yk commented 4 years ago

...or you do it much simpler: select the commit, right-click the package, and just choose "Browse edition in selected version" :-) In the Monticello snapshot browser you will see the scripts when you select nothing in the panes above.

tom95 commented 4 years ago

Alright I believe I got the behavior correct in all cases now. Instead of cancel, we now wait for windowIsClosing to also catch the user dismissing the window without clicking the Cancel button.

I changed the spec's setter selector to acceptMessage: to make it clear that this will also commit the transaction. #message: is still used by the setup code that restores the previous message.

How should I got about the inadvertent changes? Just reset them and do a cleanup commit? I am also not sure what happened on the STON output, it should be the latest version from the squeak-ston repo.

tom95 commented 4 years ago

...and the askBeforeDiscardingEdits: false is apparently a somewhat quirky way of telling the widget that we do not care about the changed state when it comes to accepting. Without setting this flag to false using an already stored string in the model cannot be accepted, e.g. when we come up with a previous commit message and the user just wants to accept without doing further changes.