Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
On April 2, 2015, 4:44 p.m., Marco Martin wrote: wallpapers/image/backgroundlistmodel.h, line 93 https://git.reviewboard.kde.org/r/123211/diff/2/?file=359798#file359798line93 this function is not clear what it actually does looks like a verb but returns a list? i tought it was actually *doing* the deletion, maybe either this could directly call removewallpaper or be renamed to something else like wallpapersAwaitingDeletion() or deletionCandidates() yes, the name of that function was a bit off. So i rename it to wallpapersAwaitingDeletion(). - Antonis --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/#review78416 --- On April 3, 2015, 3:06 p.m., Antonis Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 3, 2015, 3:06 p.m.) Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/backgroundlistmodel.h 01b528d wallpapers/image/backgroundlistmodel.cpp d062650 wallpapers/image/image.h a00e431 wallpapers/image/image.cpp f0c672a wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png wallpaper remove https://git.reviewboard.kde.org/media/uploaded/files/2015/04/02/8acc3dab-4c2a-4cc2-9ba3-9893e42c32d3__wallpaper_undo.png wallpaper restore https://git.reviewboard.kde.org/media/uploaded/files/2015/04/02/4b23e100-194a-4557-926c-d7af252f15d7__wallpaper_undo3.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/#review78440 --- Ship it! Ship It! - Marco Martin On April 3, 2015, 3:06 p.m., Antonis Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 3, 2015, 3:06 p.m.) Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/backgroundlistmodel.h 01b528d wallpapers/image/backgroundlistmodel.cpp d062650 wallpapers/image/image.h a00e431 wallpapers/image/image.cpp f0c672a wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png wallpaper remove https://git.reviewboard.kde.org/media/uploaded/files/2015/04/02/8acc3dab-4c2a-4cc2-9ba3-9893e42c32d3__wallpaper_undo.png wallpaper restore https://git.reviewboard.kde.org/media/uploaded/files/2015/04/02/4b23e100-194a-4557-926c-d7af252f15d7__wallpaper_undo3.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/#review78441 --- It's great that you've implemented the undo function, awesome! What I cannot really tell from the screenshots is how a wallpaper which is marked to be removed is visualized. Can you maybe add a screenshot where the distinction between those which are marked for removal and those which are not becomes clear? Thanks! - Thomas Pfeiffer On April 3, 2015, 3:06 p.m., Antonis Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 3, 2015, 3:06 p.m.) Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/backgroundlistmodel.h 01b528d wallpapers/image/backgroundlistmodel.cpp d062650 wallpapers/image/image.h a00e431 wallpapers/image/image.cpp f0c672a wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png wallpaper remove https://git.reviewboard.kde.org/media/uploaded/files/2015/04/02/8acc3dab-4c2a-4cc2-9ba3-9893e42c32d3__wallpaper_undo.png wallpaper restore https://git.reviewboard.kde.org/media/uploaded/files/2015/04/02/4b23e100-194a-4557-926c-d7af252f15d7__wallpaper_undo3.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
On April 3, 2015, 3:13 p.m., Thomas Pfeiffer wrote: It's great that you've implemented the undo function, awesome! What I cannot really tell from the screenshots is how a wallpaper which is marked to be removed is visualized. Can you maybe add a screenshot where the distinction between those which are marked for removal and those which are not becomes clear? Thanks! I have add a new screenshot. The wallpaper with the kde forums is currenty being marked for deletion, while the one with the bugzilla it is not being marked for deletion. The basic difference between the deleted and the non deleted is that i add opacity. So the one which is going to be deleted is shown up as faded out.. I am pushing this patch as is for now. If you have any better ideas, about how we could archive a better distinction between these two, let me know. - Antonis --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/#review78441 --- On April 3, 2015, 3:58 p.m., Antonis Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 3, 2015, 3:58 p.m.) Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/backgroundlistmodel.h 01b528d wallpapers/image/backgroundlistmodel.cpp d062650 wallpapers/image/image.h a00e431 wallpapers/image/image.cpp f0c672a wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png wallpaper remove https://git.reviewboard.kde.org/media/uploaded/files/2015/04/02/8acc3dab-4c2a-4cc2-9ba3-9893e42c32d3__wallpaper_undo.png wallpaper restore https://git.reviewboard.kde.org/media/uploaded/files/2015/04/02/4b23e100-194a-4557-926c-d7af252f15d7__wallpaper_undo3.png 1st_is_removed_2nd_isnt https://git.reviewboard.kde.org/media/uploaded/files/2015/04/03/84e524f9-7875-42b2-ad8c-3083b7e0bd22__1st_wallapper_is_removed_the_2nd_isnt.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 3, 2015, 4:05 p.m.) Status -- This change has been marked as submitted. Review request for Plasma. Changes --- Submitted with commit 50942e2618f3ed26f8eac8f264656617bb113ec6 by Antonis Tsiapaliokas to branch master. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/backgroundlistmodel.h 01b528d wallpapers/image/backgroundlistmodel.cpp d062650 wallpapers/image/image.h a00e431 wallpapers/image/image.cpp f0c672a wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png wallpaper remove https://git.reviewboard.kde.org/media/uploaded/files/2015/04/02/8acc3dab-4c2a-4cc2-9ba3-9893e42c32d3__wallpaper_undo.png wallpaper restore https://git.reviewboard.kde.org/media/uploaded/files/2015/04/02/4b23e100-194a-4557-926c-d7af252f15d7__wallpaper_undo3.png 1st_is_removed_2nd_isnt https://git.reviewboard.kde.org/media/uploaded/files/2015/04/03/84e524f9-7875-42b2-ad8c-3083b7e0bd22__1st_wallapper_is_removed_the_2nd_isnt.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/#review78450 --- wallpapers/image/backgroundlistmodel.h (line 93) https://git.reviewboard.kde.org/r/123211/#comment53671 This should be: QStringList wallpapersAwaitingDeletion() const; wallpapers/image/image.cpp (line 802) https://git.reviewboard.kde.org/r/123211/#comment53672 This should be a const reference: const QString wallpaperCandidate - Lukáš Tinkl On Dub. 3, 2015, 6:05 odp., Antonis Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated Dub. 3, 2015, 6:05 odp.) Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/backgroundlistmodel.h 01b528d wallpapers/image/backgroundlistmodel.cpp d062650 wallpapers/image/image.h a00e431 wallpapers/image/image.cpp f0c672a wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png wallpaper remove https://git.reviewboard.kde.org/media/uploaded/files/2015/04/02/8acc3dab-4c2a-4cc2-9ba3-9893e42c32d3__wallpaper_undo.png wallpaper restore https://git.reviewboard.kde.org/media/uploaded/files/2015/04/02/4b23e100-194a-4557-926c-d7af252f15d7__wallpaper_undo3.png 1st_is_removed_2nd_isnt https://git.reviewboard.kde.org/media/uploaded/files/2015/04/03/84e524f9-7875-42b2-ad8c-3083b7e0bd22__1st_wallapper_is_removed_the_2nd_isnt.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 3, 2015, 3:58 p.m.) Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/backgroundlistmodel.h 01b528d wallpapers/image/backgroundlistmodel.cpp d062650 wallpapers/image/image.h a00e431 wallpapers/image/image.cpp f0c672a wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments (updated) dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png wallpaper remove https://git.reviewboard.kde.org/media/uploaded/files/2015/04/02/8acc3dab-4c2a-4cc2-9ba3-9893e42c32d3__wallpaper_undo.png wallpaper restore https://git.reviewboard.kde.org/media/uploaded/files/2015/04/02/4b23e100-194a-4557-926c-d7af252f15d7__wallpaper_undo3.png 1st_is_removed_2nd_isnt https://git.reviewboard.kde.org/media/uploaded/files/2015/04/03/84e524f9-7875-42b2-ad8c-3083b7e0bd22__1st_wallapper_is_removed_the_2nd_isnt.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 2, 2015, 4:24 p.m.) Review request for Plasma. Changes --- This patch is implementing an undo feature for the deleted wallpapers. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs (updated) - wallpapers/image/backgroundlistmodel.h 01b528d wallpapers/image/backgroundlistmodel.cpp d062650 wallpapers/image/image.h a00e431 wallpapers/image/image.cpp f0c672a wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments (updated) dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png wallpaper remove https://git.reviewboard.kde.org/media/uploaded/files/2015/04/02/8acc3dab-4c2a-4cc2-9ba3-9893e42c32d3__wallpaper_undo.png wallpaper restore https://git.reviewboard.kde.org/media/uploaded/files/2015/04/02/4b23e100-194a-4557-926c-d7af252f15d7__wallpaper_undo3.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/#review78416 --- I think it's on the right track, biggest complain is the neaming of the methods so far wallpapers/image/backgroundlistmodel.h (line 92) https://git.reviewboard.kde.org/r/123211/#comment53651 setPendingDeletion()? wallpapers/image/backgroundlistmodel.h (line 93) https://git.reviewboard.kde.org/r/123211/#comment53652 this function is not clear what it actually does looks like a verb but returns a list? i tought it was actually *doing* the deletion, maybe either this could directly call removewallpaper or be renamed to something else like wallpapersAwaitingDeletion() or deletionCandidates() - Marco Martin On April 2, 2015, 4:24 p.m., Antonis Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 2, 2015, 4:24 p.m.) Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/backgroundlistmodel.h 01b528d wallpapers/image/backgroundlistmodel.cpp d062650 wallpapers/image/image.h a00e431 wallpapers/image/image.cpp f0c672a wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png wallpaper remove https://git.reviewboard.kde.org/media/uploaded/files/2015/04/02/8acc3dab-4c2a-4cc2-9ba3-9893e42c32d3__wallpaper_undo.png wallpaper restore https://git.reviewboard.kde.org/media/uploaded/files/2015/04/02/4b23e100-194a-4557-926c-d7af252f15d7__wallpaper_undo3.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request 123211: Ask for confirmation before deleting a wallpaper.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/#review78348 --- wallpapers/image/imagepackage/contents/ui/config.qml (line 251) https://git.reviewboard.kde.org/r/123211/#comment53636 the problem in using a framesvgitem mixed with QtControls is that while with the default themes is ok (and i think it looks great/prefer how it looks inline this way over a separate dialog window) you would risk to have things like black text on black background with different plasma themes or desktop color schemes. - Marco Martin On April 1, 2015, 3:27 p.m., Antonis Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 1, 2015, 3:27 p.m.) Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
On April 1, 2015, 4:13 nachm., Thomas Pfeiffer wrote: I agree with Kai: Asking for confirmation in this case is not the approach we should take. What this definitely calls for is an undo function, which should certainly not be impossible to do. Asking for confirmation should only be the last resort if undoing an action is simply not possible (e.g. when deleting a file permanently). A confirmation adds another step to the 99% of actions that were intentional, only to protect from the 1% accidents. Undo, on the other hand, doesn't bother users in 99% of the cases while helping them in the 1%. Yes, an undo feature may sound overblown in this case, but we must get into the habit of moving away from confirmation dialogs and towards undo. Also, please add usability to review requests that contain a significant user interface / interaction element in the future. - Thomas --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/#review78354 --- On April 1, 2015, 3:27 nachm., Antonis Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 1, 2015, 3:27 nachm.) Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/#review78350 --- I don't think the wallpaper dialog should ever delete an actual image file, especially not when it's in the user's Pictures folder. Only exception is when it has been installed through GHNS in which case it is in some hidden dot folder. As for the confirmation, I think we should rather provide an undo functionality so you don't accidentally remove the wallpaper from the list and then have to search it on the filesystem again. - Kai Uwe Broulik On April 1, 2015, 3:27 nachm., Antonis Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 1, 2015, 3:27 nachm.) Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
On April 1, 2015, 3:58 p.m., Kai Uwe Broulik wrote: I don't think the wallpaper dialog should ever delete an actual image file, especially not when it's in the user's Pictures folder. Only exception is when it has been installed through GHNS in which case it is in some hidden dot folder. As for the confirmation, I think we should rather provide an undo functionality so you don't accidentally remove the wallpaper from the list and then have to search it on the filesystem again. I think the patch for now is fine as is, modulo using QtQuickcontrols. the file is deleted in ghns case and just the entry removed in case of photo on the hard drive. the wording on the dialog should reflect that. - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/#review78350 --- On April 1, 2015, 3:27 p.m., Antonis Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 1, 2015, 3:27 p.m.) Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/#review78347 --- The complex part of the wallpapers is in some cases it actually removes the file, sometimes it doesn't depending on whether we got it from GHNS or the user just selected a file. Do we want the dialog every time when it's a local file and no deleting actually occurs? I'm not sure either way. I certainly think we should avoid saying delete in the message when we're not deleting the file. wallpapers/image/imagepackage/contents/ui/config.qml (line 251) https://git.reviewboard.kde.org/r/123211/#comment53635 QQC label on top of a plamsa frame is very liable to be lead to a black text on black background situation. No Plamsa.Anything should ever be used in config dialogs. Maybe http://doc.qt.io/qt-5/qml-qtquick-dialogs-messagedialog.html is better? - David Edmundson On April 1, 2015, 3:27 p.m., Antonis Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 1, 2015, 3:27 p.m.) Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
On April 1, 2015, 3:58 nachm., Kai Uwe Broulik wrote: I don't think the wallpaper dialog should ever delete an actual image file, especially not when it's in the user's Pictures folder. Only exception is when it has been installed through GHNS in which case it is in some hidden dot folder. As for the confirmation, I think we should rather provide an undo functionality so you don't accidentally remove the wallpaper from the list and then have to search it on the filesystem again. Marco Martin wrote: I think the patch for now is fine as is, modulo using QtQuickcontrols. the file is deleted in ghns case and just the entry removed in case of photo on the hard drive. the wording on the dialog should reflect that. The patch may be fine from a technical perspective, but it has a considerable negative impact on user experience, and therefore should not be shipped anyway. - Thomas --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/#review78350 --- On April 1, 2015, 3:27 nachm., Antonis Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 1, 2015, 3:27 nachm.) Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/#review78346 --- wallpapers/image/imagepackage/contents/ui/config.qml (line 251) https://git.reviewboard.kde.org/r/123211/#comment53634 Please use QtQuick Controls. http://doc.qt.io/qt-5/qml-qtquick-dialogs-messagedialog.html - Aleix Pol Gonzalez On April 1, 2015, 5:27 p.m., Antonis Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 1, 2015, 5:27 p.m.) Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
On April 1, 2015, 3:58 p.m., Kai Uwe Broulik wrote: I don't think the wallpaper dialog should ever delete an actual image file, especially not when it's in the user's Pictures folder. Only exception is when it has been installed through GHNS in which case it is in some hidden dot folder. As for the confirmation, I think we should rather provide an undo functionality so you don't accidentally remove the wallpaper from the list and then have to search it on the filesystem again. Marco Martin wrote: I think the patch for now is fine as is, modulo using QtQuickcontrols. the file is deleted in ghns case and just the entry removed in case of photo on the hard drive. the wording on the dialog should reflect that. Thomas Pfeiffer wrote: The patch may be fine from a technical perspective, but it has a considerable negative impact on user experience, and therefore should not be shipped anyway. It adresses a specific bug, so no, it doesn't have a negative impact compared to now. I agree that an undo feature would be better, but I consider more productive having iterative revisions even with different approaches rather than shoot down contributors. I have a couple of ideas how an undo feature could be implemented, in this case i think it can be shown an undo button on like a greyed down wallpaper thumbnail until ok or apply is clicked, that moment the change is committed irreversibely. will discuss the implementation in another comment - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/#review78350 --- On April 1, 2015, 3:27 p.m., Antonis Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 1, 2015, 3:27 p.m.) Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/#review78354 --- I agree with Kai: Asking for confirmation in this case is not the approach we should take. What this definitely calls for is an undo function, which should certainly not be impossible to do. Asking for confirmation should only be the last resort if undoing an action is simply not possible (e.g. when deleting a file permanently). A confirmation adds another step to the 99% of actions that were intentional, only to protect from the 1% accidents. Undo, on the other hand, doesn't bother users in 99% of the cases while helping them in the 1%. Yes, an undo feature may sound overblown in this case, but we must get into the habit of moving away from confirmation dialogs and towards undo. - Thomas Pfeiffer On April 1, 2015, 3:27 nachm., Antonis Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 1, 2015, 3:27 nachm.) Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
On April 1, 2015, 4:44 nachm., Marco Martin wrote: An undo feature can be done as following: the wallpaper model would have a role like pendingDeletion that would be set by the remove button on the thumbnail. At that point the thumbnail can show an undo button based on the role. Upon apply or ok, a commitDeletion method would be called on the model instance. this would go trough all the items and do the removeWallpaper on the ones that have the PendingDeletion role set. Cancel would not delete anything. Antonis, would you feel giving a try on it? Interesting approach! All in all, I see three possible approaches here: 1. The one you described. Advantage: Users can make any modification to their decisions before they are committed Disadvantage: If only one wallpaper is removed and nothing else is changed, it still adds another action which would otherwise not be needed 2. An undo function that only applies to the most recently removed wallpaper, so really just an Oops, I didn't mean to do that, Ctrl-Z! kind of thing, similar to the Plasmoid removal undo. As soon as another one is removed or the dialog is closed, it would not be possible to undo it anymore Advantage: Adds no additional action in case one just removes one wallpaper (not by accident) and does nothing else Disadvantage: Users could still lose their wallpapers if they realize their mistake too late 3. Establishing a Trash for wallpapers which contains both the file and the entry in the database of wallpapers, which stays until actively emptied by the user Advantage: Adds no further action and it's possible to delete multiple wallpapers in a row and restore them all Disadvantage: Probably the most complex one implementation wise, plus would need a way to show and empty the trash In the end, we should look at this in the context of other areas where undo functionality makes sense and choose the approach that can be applied (at least in a similar way from a user perspective) to the broadest range of cases. The problem this solves is not important enough to create a solution just for this situation alone. It's only worth doing if it can be a model for similar situations (and there are plenty of them) - Thomas --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/#review78359 --- On April 1, 2015, 3:27 nachm., Antonis Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 1, 2015, 3:27 nachm.) Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
On April 1, 2015, 3:58 nachm., Kai Uwe Broulik wrote: I don't think the wallpaper dialog should ever delete an actual image file, especially not when it's in the user's Pictures folder. Only exception is when it has been installed through GHNS in which case it is in some hidden dot folder. As for the confirmation, I think we should rather provide an undo functionality so you don't accidentally remove the wallpaper from the list and then have to search it on the filesystem again. Marco Martin wrote: I think the patch for now is fine as is, modulo using QtQuickcontrols. the file is deleted in ghns case and just the entry removed in case of photo on the hard drive. the wording on the dialog should reflect that. Thomas Pfeiffer wrote: The patch may be fine from a technical perspective, but it has a considerable negative impact on user experience, and therefore should not be shipped anyway. Marco Martin wrote: It adresses a specific bug, so no, it doesn't have a negative impact compared to now. I agree that an undo feature would be better, but I consider more productive having iterative revisions even with different approaches rather than shoot down contributors. I have a couple of ideas how an undo feature could be implemented, in this case i think it can be shown an undo button on like a greyed down wallpaper thumbnail until ok or apply is clicked, that moment the change is committed irreversibely. will discuss the implementation in another comment Yes, it addresses the situation that people may accidentally delete a wallpaper that was downloaded from GHNS and then cannot not find it there anymore. I would not consider this problem as important enough to put all users through another conformation dialog, though, so I do not see it as an incremental step towards a better user experience. I didn't mean to criticize Antonis for that, though. I apologize if that's how it came across. According to your expert review (which I fully trust), Antonis provied a sound patch to address a bug report. The problem I have is that the usability team was informed neither of the bug nor the patch, even though both clearly had usability implications. So, Antonis did a fine job, but our usability review process still seems to be imperfect (though it's steadily improving). - Thomas --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/#review78350 --- On April 1, 2015, 3:27 nachm., Antonis Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 1, 2015, 3:27 nachm.) Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/#review78359 --- An undo feature can be done as following: the wallpaper model would have a role like pendingDeletion that would be set by the remove button on the thumbnail. At that point the thumbnail can show an undo button based on the role. Upon apply or ok, a commitDeletion method would be called on the model instance. this would go trough all the items and do the removeWallpaper on the ones that have the PendingDeletion role set. Cancel would not delete anything. Antonis, would you feel giving a try on it? - Marco Martin On April 1, 2015, 3:27 p.m., Antonis Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 1, 2015, 3:27 p.m.) Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
On April 1, 2015, 4:44 p.m., Marco Martin wrote: An undo feature can be done as following: the wallpaper model would have a role like pendingDeletion that would be set by the remove button on the thumbnail. At that point the thumbnail can show an undo button based on the role. Upon apply or ok, a commitDeletion method would be called on the model instance. this would go trough all the items and do the removeWallpaper on the ones that have the PendingDeletion role set. Cancel would not delete anything. Antonis, would you feel giving a try on it? Thomas Pfeiffer wrote: Interesting approach! All in all, I see three possible approaches here: 1. The one you described. Advantage: Users can make any modification to their decisions before they are committed Disadvantage: If only one wallpaper is removed and nothing else is changed, it still adds another action which would otherwise not be needed 2. An undo function that only applies to the most recently removed wallpaper, so really just an Oops, I didn't mean to do that, Ctrl-Z! kind of thing, similar to the Plasmoid removal undo. As soon as another one is removed or the dialog is closed, it would not be possible to undo it anymore Advantage: Adds no additional action in case one just removes one wallpaper (not by accident) and does nothing else Disadvantage: Users could still lose their wallpapers if they realize their mistake too late 3. Establishing a Trash for wallpapers which contains both the file and the entry in the database of wallpapers, which stays until actively emptied by the user Advantage: Adds no further action and it's possible to delete multiple wallpapers in a row and restore them all Disadvantage: Probably the most complex one implementation wise, plus would need a way to show and empty the trash In the end, we should look at this in the context of other areas where undo functionality makes sense and choose the approach that can be applied (at least in a similar way from a user perspective) to the broadest range of cases. The problem this solves is not important enough to create a solution just for this situation alone. It's only worth doing if it can be a model for similar situations (and there are plenty of them) talking from implementation standpoint 1) would be trivial, 2 and 3 more of a mess (not sure woth the bother doing it for such a minor use case) as consistency, will always be for the situation in particular alone, every single case is completely different in implementation and i think in what could be shown to the user as well - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/#review78359 --- On April 1, 2015, 3:27 p.m., Antonis Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 1, 2015, 3:27 p.m.) Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
On April 1, 2015, 4:44 p.m., Marco Martin wrote: An undo feature can be done as following: the wallpaper model would have a role like pendingDeletion that would be set by the remove button on the thumbnail. At that point the thumbnail can show an undo button based on the role. Upon apply or ok, a commitDeletion method would be called on the model instance. this would go trough all the items and do the removeWallpaper on the ones that have the PendingDeletion role set. Cancel would not delete anything. Antonis, would you feel giving a try on it? Thomas Pfeiffer wrote: Interesting approach! All in all, I see three possible approaches here: 1. The one you described. Advantage: Users can make any modification to their decisions before they are committed Disadvantage: If only one wallpaper is removed and nothing else is changed, it still adds another action which would otherwise not be needed 2. An undo function that only applies to the most recently removed wallpaper, so really just an Oops, I didn't mean to do that, Ctrl-Z! kind of thing, similar to the Plasmoid removal undo. As soon as another one is removed or the dialog is closed, it would not be possible to undo it anymore Advantage: Adds no additional action in case one just removes one wallpaper (not by accident) and does nothing else Disadvantage: Users could still lose their wallpapers if they realize their mistake too late 3. Establishing a Trash for wallpapers which contains both the file and the entry in the database of wallpapers, which stays until actively emptied by the user Advantage: Adds no further action and it's possible to delete multiple wallpapers in a row and restore them all Disadvantage: Probably the most complex one implementation wise, plus would need a way to show and empty the trash In the end, we should look at this in the context of other areas where undo functionality makes sense and choose the approach that can be applied (at least in a similar way from a user perspective) to the broadest range of cases. The problem this solves is not important enough to create a solution just for this situation alone. It's only worth doing if it can be a model for similar situations (and there are plenty of them) Marco Martin wrote: talking from implementation standpoint 1) would be trivial, 2 and 3 more of a mess (not sure woth the bother doing it for such a minor use case) as consistency, will always be for the situation in particular alone, every single case is completely different in implementation and i think in what could be shown to the user as well Thomas Pfeiffer wrote: Do you think it would be theoretically possible at least to apply 1) to other KCMs where themes etc. can be installed or removed (at least from an interaction perspective, even if the underlying implememtation would differ)? Accidentally removing e.g. a Plasma theme would be exactly as good or bad as accidentally removing a wallpaper which was downloaded thorugh GHNS, after all. yes, I think so. basically, since we can't really undo, what would actually do is to fake the removal of the theme or the thing in question and actually do it on config apply (like for plasmoids at the moment, is faking removal as well) a similar trick should be employable in several places - Marco --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/#review78359 --- On April 1, 2015, 3:27 p.m., Antonis Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 1, 2015, 3:27 p.m.) Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
On April 1, 2015, 4:44 p.m., Marco Martin wrote: An undo feature can be done as following: the wallpaper model would have a role like pendingDeletion that would be set by the remove button on the thumbnail. At that point the thumbnail can show an undo button based on the role. Upon apply or ok, a commitDeletion method would be called on the model instance. this would go trough all the items and do the removeWallpaper on the ones that have the PendingDeletion role set. Cancel would not delete anything. Antonis, would you feel giving a try on it? Thomas Pfeiffer wrote: Interesting approach! All in all, I see three possible approaches here: 1. The one you described. Advantage: Users can make any modification to their decisions before they are committed Disadvantage: If only one wallpaper is removed and nothing else is changed, it still adds another action which would otherwise not be needed 2. An undo function that only applies to the most recently removed wallpaper, so really just an Oops, I didn't mean to do that, Ctrl-Z! kind of thing, similar to the Plasmoid removal undo. As soon as another one is removed or the dialog is closed, it would not be possible to undo it anymore Advantage: Adds no additional action in case one just removes one wallpaper (not by accident) and does nothing else Disadvantage: Users could still lose their wallpapers if they realize their mistake too late 3. Establishing a Trash for wallpapers which contains both the file and the entry in the database of wallpapers, which stays until actively emptied by the user Advantage: Adds no further action and it's possible to delete multiple wallpapers in a row and restore them all Disadvantage: Probably the most complex one implementation wise, plus would need a way to show and empty the trash In the end, we should look at this in the context of other areas where undo functionality makes sense and choose the approach that can be applied (at least in a similar way from a user perspective) to the broadest range of cases. The problem this solves is not important enough to create a solution just for this situation alone. It's only worth doing if it can be a model for similar situations (and there are plenty of them) Marco Martin wrote: talking from implementation standpoint 1) would be trivial, 2 and 3 more of a mess (not sure woth the bother doing it for such a minor use case) as consistency, will always be for the situation in particular alone, every single case is completely different in implementation and i think in what could be shown to the user as well Thomas Pfeiffer wrote: Do you think it would be theoretically possible at least to apply 1) to other KCMs where themes etc. can be installed or removed (at least from an interaction perspective, even if the underlying implememtation would differ)? Accidentally removing e.g. a Plasma theme would be exactly as good or bad as accidentally removing a wallpaper which was downloaded thorugh GHNS, after all. Marco Martin wrote: yes, I think so. basically, since we can't really undo, what would actually do is to fake the removal of the theme or the thing in question and actually do it on config apply (like for plasmoids at the moment, is faking removal as well) a similar trick should be employable in several places @Marco Yes, I will work on it tomorrow. - Antonis --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/#review78359 --- On April 1, 2015, 3:27 p.m., Antonis Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 1, 2015, 3:27 p.m.) Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 123211: Ask for confirmation before deleting a wallpaper.
On April 1, 2015, 4:44 nachm., Marco Martin wrote: An undo feature can be done as following: the wallpaper model would have a role like pendingDeletion that would be set by the remove button on the thumbnail. At that point the thumbnail can show an undo button based on the role. Upon apply or ok, a commitDeletion method would be called on the model instance. this would go trough all the items and do the removeWallpaper on the ones that have the PendingDeletion role set. Cancel would not delete anything. Antonis, would you feel giving a try on it? Thomas Pfeiffer wrote: Interesting approach! All in all, I see three possible approaches here: 1. The one you described. Advantage: Users can make any modification to their decisions before they are committed Disadvantage: If only one wallpaper is removed and nothing else is changed, it still adds another action which would otherwise not be needed 2. An undo function that only applies to the most recently removed wallpaper, so really just an Oops, I didn't mean to do that, Ctrl-Z! kind of thing, similar to the Plasmoid removal undo. As soon as another one is removed or the dialog is closed, it would not be possible to undo it anymore Advantage: Adds no additional action in case one just removes one wallpaper (not by accident) and does nothing else Disadvantage: Users could still lose their wallpapers if they realize their mistake too late 3. Establishing a Trash for wallpapers which contains both the file and the entry in the database of wallpapers, which stays until actively emptied by the user Advantage: Adds no further action and it's possible to delete multiple wallpapers in a row and restore them all Disadvantage: Probably the most complex one implementation wise, plus would need a way to show and empty the trash In the end, we should look at this in the context of other areas where undo functionality makes sense and choose the approach that can be applied (at least in a similar way from a user perspective) to the broadest range of cases. The problem this solves is not important enough to create a solution just for this situation alone. It's only worth doing if it can be a model for similar situations (and there are plenty of them) Marco Martin wrote: talking from implementation standpoint 1) would be trivial, 2 and 3 more of a mess (not sure woth the bother doing it for such a minor use case) as consistency, will always be for the situation in particular alone, every single case is completely different in implementation and i think in what could be shown to the user as well Do you think it would be theoretically possible at least to apply 1) to other KCMs where themes etc. can be installed or removed (at least from an interaction perspective, even if the underlying implememtation would differ)? Accidentally removing e.g. a Plasma theme would be exactly as good or bad as accidentally removing a wallpaper which was downloaded thorugh GHNS, after all. - Thomas --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/#review78359 --- On April 1, 2015, 3:27 nachm., Antonis Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123211/ --- (Updated April 1, 2015, 3:27 nachm.) Review request for Plasma. Bugs: 338729 https://bugs.kde.org/show_bug.cgi?id=338729 Repository: plasma-workspace Description --- This patch is adding a confirmation dialog which is being called before we remove a wallpaper. Diffs - wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml aee2d3f wallpapers/image/imagepackage/contents/ui/config.qml 2108082 Diff: https://git.reviewboard.kde.org/r/123211/diff/ Testing --- File Attachments dialog https://git.reviewboard.kde.org/media/uploaded/files/2015/04/01/5bfd2d7c-8baa-4b80-ad20-0844aafdb3a9__deletion_dialog.png Thanks, Antonis Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
