dfaure added a comment.
In https://phabricator.kde.org/D7270#136443, @chinmoyr wrote: > > Is this subclass needed? You could just move the code of its constructor to the UndoJob constructor, no? (using Q_D of course) > > I tried this before anything else and it didn't worked. Q_D requires a private class, right? Ah, good point, this would require a manual use of d_func() instead, to get to the private class of the base class. Here's how Qt does it in a few places: static_cast<BaseClass *>(d_func()). >> It seems to me that we only want to undo with "privilege execution enabled" if the original job has privilege execution enabled, no? >> This requires storing that information together with the undo information... > > If original job has the flag set and privilege operation succeeds, undo will be available and if the flag is not set then no operation will take place and undo won't be available. > So I don't think storing any other info is required. In Dolphin (for instance), all jobs triggered by the user will have the flag set, even those which didn't actually need privilege operations, right? So how can FileUndoManager make a difference between a file-copy in my home (no privilege needed, neither for the operation nor for the undo) and a file-copy into /etc (root privilege needed, both for the operation and for undo)? But yeah, I'm still undecided between two solutions: 1. only attempt privilege elevation at undo time if it was needed for the initial job 2. just like any other kio job, use privilege elevation after trying without, if that failed -- even if the initial job didn't need that. I can come up with reverse examples: `cp /root/.bashrc /tmp` : the initial operation needs root (to read the file), while undo doesn't need root (anyone can delete files in /tmp). I can't come up with an example where the initial operation doesn't need root but undo does... > I added the variable so that it can be toggled through a dolphin setting. My initial plan was to add a checkbox/toggle to dolphin's setting to "Enable/Disable undo in read-only folder" and read it in FileUndoManager. That time I assumed some users might want to disable undo inside a read-only folder. Personally I want Undo to be there all the time. Do you think there is any need to add such setting? Otherwise that variable can be removed and some of the issues you pointed will be solved as well. I'm pretty sure that this is "over-configurability". *Maybe* someone wants to disable the whole support for root prompts (e.g. to prevent their grandma from deleting files in /etc, on systems where root has the same password as user...), but I definitely don't see a use case for a checkbox that would be specifically about the undo support and only that. REVISION DETAIL https://phabricator.kde.org/D7270 To: chinmoyr, #frameworks, dfaure
