What to do about KShortcutsEditor and undo

[Andreas Hartmetz]

2008/5/20 Michael Jansen <kde at michael-jansen.biz>:
> On Tuesday 20 May 2008 02:49:59 Andreas Hartmetz wrote:
>> Hi all,
>>
>> -Add setUndoCheckpoint() to KShortcutsEditor so that KControl modules can
>> call it when asked to save configuration. Calling KShortcutsEditor::undo()
>> will then only undo to the state at the checkpoint.
>
> @see KShortcutsEditor::save() and KShortcutsEditor::undChanges and tell me
> what additional functionality your methods provide. They do exactly what you
> propose afaict.
>
Oops, I missed save(). save() is fine. I assume the implementation is fine, too.

> What is the correct behaviour for the dialog upon destruction when there are
> unsaved changes? /me thinks undoing the changes. That's what i implemented.
>
Due to parents deleting childs, and in principle as well, it's a bad
idea in library code to do anything nontrivial involving objects you
don't own in the destructor.

>> -Remove automatic undo in KShortcutsEditor on destruction, it's not a very
>>  good idea if only for the reason that it crashes if the actions are
>> destroyed before the editor. This actually happens in the KWin effects
>> config modules.
>
> If you provide a something (the collection)  to another object (the editor)
> it's your responsibilty to ensure it's valid as long as the object lives. If
> you destroy the something as long as the object lives we enter the stage of
> undefined behaviour. So i see no problem here. The problem is the way the kwin
> effects use the kshortcutsdialog.
>
See above, the code of the KWin effects module will crash if written
the obvious way. This is really the killer argument, the others don't
matter nearly as much. You would have to take special measures to
avoid the crash and I really really don't want to force that upon API
users. A forgotten undo() would have less dramatic consequences and
its purpose would be very obvious, leading to code that is easier to
understand. Understand as in "know what every single line is good for,
without comments".

>> KControl modules should call undo() on their destruction
>> when the actions are still alive.
>
> If they would do that now ... no core dump. But that's just luck. They still
> destroy an object given to another object to work on without making sure they
> get destroyed in the right order.
>
Caring about destruction order is not a productive way to spend time.

>> -Remove (if no protests, otherwise deprecate)
>>  KGlobalAccel::overrideComponentData(). It was conceived as a special-case
>>  hack for KControl modules (which configure actions that they don't "own");
>>  KActionCollection::setComponentData() can replace it and it will be more
>>  consistent and generally applicable. Credit to mjansen for pointing this
>> out. overrideComponentData() is only used in kdelibs and kdebase, according
>> to lxr.kde.org.
>
> That has nothing to do with the problem at hand. But i'm all in for removing
> it. Btw. i told you if we can agree on the patch i send to you i would fix the
> kwin effects. There's nothing else needed. The rest is just to use the api
> correctly.
>
Actually I took a whack at fixing kwin effects (in order to test the
new way of doing things) and noticed that automatic undo is difficult
to handle. The difference is like this:
- automatic undo: need to delete the KShortcutsEditor first to ensure
that it runs its undo before the actions are destroyed. Or do
something to deterministically ensure that the actions will stay
around longer. But you also should never delete QObjects using delete
because it's dangerous, too. If you use the safer deleteLater() the
order of destruction is unsure again, depending on when you return to
the event loop. Or, in a nutshell: You need to take care of details
that have nothing to do with application logic.
- no automatic undo: call undo() on the KShortcutsEditor in the
destructor. As this is the "top-level" destructor there shouldn't be
any bad surprises.