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

undo could also work with properties window #856

Open
jmmmp opened this issue Dec 31, 2019 · 18 comments
Open

undo could also work with properties window #856

jmmmp opened this issue Dec 31, 2019 · 18 comments

Comments

@jmmmp
Copy link

@jmmmp jmmmp commented Dec 31, 2019

The recent undo changes are very helpful. It would be even more interesting if the settings changes made in all properties windows would be accounted for.
Here is what I could find:

  • undo works for canvas properties (patch, not cnv) if used with right-click on the background, the entry "undo apply" appears
  • it doesn't work for the same dialogue if right-clicked on a subpatch e.g. [pd test], there is no undo entry either in the main patch or in the subpatch
  • all GUI properties don't count for undo.
    (- undo doesn't work with messages sent to the canvas (clear, add objects, ...) - but I myself don't have a problem with that)
@umlaeute

This comment has been minimized.

Copy link
Contributor

@umlaeute umlaeute commented Dec 31, 2019

all GUI properties don't count for undo.

that's a side-effect of undo not undoing the internal state of arbitrary objects (which is intentional).

undo really only applies to patching (canvas-properties are in a somewhat grey area (with canvases being the "objects" where the actual patching happens, and therefore they part of the undo-queue)

i don't see any proper solution for this and am inclined to close this as wontfix

@umlaeute

This comment has been minimized.

Copy link
Contributor

@umlaeute umlaeute commented Jan 1, 2020

it doesn't work for the same dialogue if right-clicked on a subpatch

this i cannot reproduce. i get an Undo action on the subpatch-canvas (just as i get when calling the dialog from the subpatch-canvas itself).

this is how i do it:

  • start with a patch that contains an empty [pd test] subpatch.
  • right-click on [pd test] and select Properties
  • enable the Graph-on-Parent flag and click Apply
    • the [pd test] is now a GOP
  • open [pd test] (right-clicking it and selecting Open)
  • call undo with Control+z on the test canvas.
    • the [pd test] is back to normal (non-GOP)

this is with Pd-0.50.2 on Debian/GNU linux.

@jmmmp

This comment has been minimized.

Copy link
Author

@jmmmp jmmmp commented Jan 2, 2020

What I meant to say was to click inside the subpatch - it seems that operations in the parent canvas are saved, but in the sub-patches they aren't.

Regarding object settings:
If undoing the internal state of arbitrary objects couldn't be done for technical reasons, I would have no problem to say "there are more important things to do, and it's unsolvable undtil tck/tk improves or [insert issue here]". But I don't see a difference between "patching" and "not-really-serious-patching", specially when:

  • all objects have send and receive settings which are vital for a patch to work (it's illogical to consider patching to add a [r xxx] above the object, but not inside it, although both have the same result)
  • when preparing a GUI, all the internal "aesthetic" settings are as important as any other. E.g. it's considered "patching" to move an object around so that it doesn't clash with other objects, but it's not patching if the size of that object is improved (which has the same effect in the end, not to clash with other objects)

I don't see much of a grey area, as what is visible from the outside is as important for the person patching as it is the content inside. Even a color scheme is important for a patch to work, although it doesn't calculate any dsp.

(This could be also stretched to dynamic patching with internal messages, but I have no issue if someone says "that's irrealistic and hard to do, and it will crash all the time")

@Spacechild1

This comment has been minimized.

Copy link
Contributor

@Spacechild1 Spacechild1 commented Jan 2, 2020

What I meant to say was to click inside the subpatch - it seems that operations in the parent canvas are saved, but in the sub-patches they aren't.

I'm not sure I understand. Can you show the exact problem?

Regarding object settings:

I agree that it's counter-intuitive that changes from the properties dialogs (both GUI and canvas properties) don't go to the undo queue.

@Spacechild1

This comment has been minimized.

Copy link
Contributor

@Spacechild1 Spacechild1 commented Jan 2, 2020

@umlaeute

i don't see any proper solution for this and am inclined to close this as wontfix

I imagine we could add a generic undo type for objects in a glist whose data is just two binbufs containing the message(s) for undo/redo and the index of the object in the list. AFAICT, undoing/redoing object creation/destruction preserves the object positions within the glist, so saving the index makes sure that the message(s) always go to the right target.

This could even be exposed in the API for custom GUI externals. I'm not saying it's a must have, but it could be a nice addition. What do you think?

@jmmmp

This comment has been minimized.

Copy link
Author

@jmmmp jmmmp commented Jan 2, 2020

I'm not sure I understand. Can you show the exact problem?

I can't get it myself anymore - with that I notice that undo seems to be canvas-related, and probably I was looking at the wrong canvas or something. It might be not very intuitive if someone is retracing steps in programming on several canvases simultaneously and isn't fully aware of this. But also a centralized undo has its own perks for a patch that is active on many fronts, so I don't know exactly what is better here.

I agree that it's counter-intuitive that changes from the properties dialogs (both GUI and canvas properties) don't go to the undo queue.

If it would be a technical issue or just too much work, I have no problem. If it's a design decision, it seems a bit arbitrary.

@porres

This comment has been minimized.

Copy link
Contributor

@porres porres commented Jan 3, 2020

Hi, I was checking on this and just realized that when you change GUI's properties on the properties window, the changes are applied immediately. I remember it didn't use to be like this and I guess I remember when it changed. I'm assuming this was intentional and for some reason I don't know/remember. The thing is that if you make a change you didn't really want to, you can't undo it. For example, you spent a good amount of time fine tuning a nice color you liked, then accidentally changed it and now it's lost!

Also, the way things are now, the cancel button in the properties window isn't really good for anything, because changes are automatically reflected on the GUI. That also makes the 'ok' button pointless.

If we can't make undo revert the changes, maybe we could use the cancel button to revert them, then the 'cancel' and 'ok' buttons would make sense.

@umlaeute

This comment has been minimized.

Copy link
Contributor

@umlaeute umlaeute commented Jan 3, 2020

(i'm not trying to defend anything, mainly trying to recap the decision-making process, not even done by me)

the behaviour @porres describes is due to the macOS design principles, where changes in property dialogs take effect immediately.
therefore this behaviour is only present on macOS (not on linux, windows or FreeBSD). the cancel button shouldn't be there on this system at all...
also i don't think that you can undo other (eg system-)properties on macOS.

@porres

This comment has been minimized.

Copy link
Contributor

@porres porres commented Jan 3, 2020

well, I see now that the truth is that this is only for the color scheme in macOS, so 'ok' and 'cancel' still make sense for the other things - sorry for the noise

@umlaeute

This comment has been minimized.

Copy link
Contributor

@umlaeute umlaeute commented Jan 3, 2020

i guess the behaviour should be consistent (that is: both color-scheme and other properties are applied immediately xor not)

umlaeute added a commit that referenced this issue Jan 3, 2020
if the object has a single method to set it's internal state,
it can call pd_undo_set_objectstate() which will call back to this method
for undoing/redoing.
when calling pd_undo_set_objectstate() we have to provide the payload
to set the state before/after the state-change

Closes: #856
@umlaeute

This comment has been minimized.

Copy link
Contributor

@umlaeute umlaeute commented Jan 7, 2020

for what it is worth: #858 implements undo/redo for the properties of iemguis and numberboxes/symbolboxes.

are there any other properties that i've missed and which should be undoable?

@umlaeute

This comment has been minimized.

Copy link
Contributor

@umlaeute umlaeute commented Jan 7, 2020

for what it is worth: #858 implements

this is of course a call-for-testing: if you have compile-powers, please checkout the PR and see whether it does what you expect.

@danomatika

This comment has been minimized.

Copy link
Contributor

@danomatika danomatika commented Jan 10, 2020

the behaviour @porres describes is due to the macOS design principles, where changes in property dialogs take effect immediately.
therefore this behaviour is only present on macOS (not on linux, windows or FreeBSD). the cancel button shouldn't be there on this system at all...

Yes, I added this only for macOS, but it could be enabled for the other platforms.

This issue was pointed after after I did the initial work and we basically would need to cache the initial object state (I'm thinking on the TCL side) when the dialog was opened, then reapply it if the cancel button was pressed. It would also be good to have the undo action only be applied when the dialog was closed and not during a "hot update" when the dialog is open.

@umlaeute

This comment has been minimized.

Copy link
Contributor

@umlaeute umlaeute commented Jan 10, 2020

Yes, I added this only for macOS, but it could be enabled for the other platforms.

i don't think that this would be a good idea.
as was discussed previously: on other platforms, dialogs usually have an Apply-button that send all requested changes at once. The notion of a properties-window that will apply each change immediately is uncommon on these platforms

i agree with the Cancel-idea.

@danomatika

This comment has been minimized.

Copy link
Contributor

@danomatika danomatika commented Jan 10, 2020

@umlaeute

This comment has been minimized.

Copy link
Contributor

@umlaeute umlaeute commented Jan 10, 2020

oh sure. but on windows & linux we use Apply (which applies the current settings while leaving the dialog open, as opposed do OK which applies the settings and closes the dialog)

@danomatika

This comment has been minimized.

Copy link
Contributor

@danomatika danomatika commented Jan 10, 2020

@umlaeute

This comment has been minimized.

Copy link
Contributor

@umlaeute umlaeute commented Jan 10, 2020

not sure i can parse this :-)

anyhow, i'm only saying: i'm sure it's nice to see changes immediately, but the way this is done on macOS (in general) is different from how it is done on other platforms, and i'd rather have my chosen platform to behave consistently than have different application pick different behaviours (if i want my computer to behave like macOS, i probably should get me a mac)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.