Review Request: Rename all files or folders and create "apply to all" checkbox in KIO rename dialog

Todd toddrme2178 at gmail.com
Sun Nov 29 01:40:12 GMT 2009 


> On 2009-11-28 21:20:46, Christoph Feck wrote:
> > /trunk/KDE/kdelibs/kio/kio/renamedialog.cpp, line 407
> > <http://reviewboard.kde.org/r/2281/diff/3/?file=15091#file15091line407>
> >
> >     Indentation is four spaces in kdelibs. Since this is a new function, there is no need to look at other functions to copy a coding style from.
> >     
> >     I won't add separate comments for the other coding style related issues, there are still braces placed below the "if" or "for" line, inconsistencies in spacing around parenthesis, and trailing white space (you can spot them using this reviewboard's diff viewer).

I think I fixed all of these.  Please let me know if I missed any.


> On 2009-11-28 21:20:46, Christoph Feck wrote:
> > /trunk/KDE/kdelibs/kio/kio/renamedialog.h, line 105
> > <http://reviewboard.kde.org/r/2281/diff/3/?file=15090#file15090line105>
> >
> >     Missing @since tag for new public function, if this needs to be public at all?

I added this


> On 2009-11-28 21:20:46, Christoph Feck wrote:
> > /trunk/KDE/kdelibs/kio/kio/copyjob.cpp, line 872
> > <http://reviewboard.kde.org/r/2281/diff/3/?file=15088#file15088line872>
> >
> >     There is a trailing ";" after the brace.

I fixed this


> On 2009-11-28 21:20:46, Christoph Feck wrote:
> > /trunk/KDE/kdelibs/kio/kio/renamedialog.h, line 54
> > <http://reviewboard.kde.org/r/2281/diff/3/?file=15090#file15090line54>
> >
> >     This is a binary incompatible change. You can only add enum values, not change them.
> >     
> >     RenameDialog is a public exported class.

I think I fixed this (I changed all of the existing numbers back to their original values, then changed R_AUTO_RENAME to 8).


On 2009-11-28 21:20:46, Todd wrote:
> > Further comments:
> > 
> > As discussed on #kde-devel, I am not really sure that adding a checkbox next to the buttons is a good thing to do, think about conversion to QDialogButtonBox later. I understand your rationale, but it looks unusual. I would like to get feedback from others here, so maybe add "usability" and "Dolphin" to review groups. Moving the checkbox to a different place would introduce a string change (also see below), but I could accept the current position as an interim solution for 4.4.
> > 
> > I have not tested the patch yet. I hope you have considered remote URLs, too.
> > 
> > It's a bit unfortunate that you request review on the message freeze day; you need to coordinate with the translation team (kde-i18n-doc list) if this can get approved for 4.4. Remember that your change does not only add a message string, but also changes the way the dialog works, so probably parts of the documentation text has be reworded accordingly.
> > 
> > Having the patch available or discussed a few weeks earlier would be a good thing for the future.

The old solution was to have a button for each action, but this was getting too long.  On the kde-core-devel mailing list people seemed to think a check box was a good solution (see the discussion with the subject "Automatic rename in KIO rename dialog").

I would have preferred to submit the patch earlier as well, but I got started late and it took longer than I anticipated both to set up the build environment and debug the patch.  I will try to submit earlier in the future.


- Todd


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/2281/#review3323
-----------------------------------------------------------


On 2009-11-29 01:33:13, Todd wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2281/
> -----------------------------------------------------------
> 
> (Updated 2009-11-29 01:33:13)
> 
> 
> Review request for Dolphin, kdelibs and usability.
> 
> 
> Summary
> -------
> 
> This patch makes two changes to the KIO file rename dialog box (the dialog box that appears when there is a file or folder name conflict when moving or copying files).  First, it adds the ability to automatically rename the files using the default file renaming rules (what you would get if you clicked "Suggest Name" once).  This is useful to avoid data loss, since you will not overwrite duplicate files or folders.
> 
> Second, because there are already a lot of possible buttons in the dialog box (rename, suggest name, overwrite, overwrite all, skip, auto skip, resume, resume all, and cancel), and since this patch would add another button, this patch eliminates "overwrite all", "auto skip", and "resume all" buttons and adds a single "apply to all" check box.  This check box changes each single-file button action to its corresponding multi-file action.  So when checked, "overwrite" behaves as "overwrite all", "skip" behaves as "auto skip", etc.  So despite the increased functionality this leads to a net reduction of up to 2 buttons, depending on the circumstances.
> 
> I know this is very late, but I think it is an important feature since there are several bug reports requesting this.  This is my first submission to KDE, so I apologize beforehand if the patch is not perfect.  I tried to follow KDE style guidelines, but there may be some commonly accepted conventions I am not aware of. 
> 
> Edit: Per a suggestion I am adding the bug reports this idea addresses:
> 
> There are three bug reports I have been able to find in a fairly quick search requesting this feature.  The first one is actually mine, the other two are from other people.  Two at least have multiple votes, so I am not the only one to have noticed this problem.  :
> 
> https://bugs.kde.org/show_bug.cgi?id=177121
> https://bugs.kde.org/show_bug.cgi?id=100769
> https://bugs.kde.org/show_bug.cgi?id=165398
> 
> Further, I have seen numerous requests for improvements to this dialog box, but unfortunately without any specifics about what to improve.  So although I can't say for certain this is what those people wanted, there are many people dissatisfied with the dialog box overall.
> 
> There was also a discussion on the mailing list where the conclusion seemed to be that this is a good idea (see "Automatic rename in KIO rename dialog")
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/kio/kio/copyjob.cpp 1053922 
>   /trunk/KDE/kdelibs/kio/kio/jobuidelegate.cpp 1053922 
>   /trunk/KDE/kdelibs/kio/kio/renamedialog.h 1053922 
>   /trunk/KDE/kdelibs/kio/kio/renamedialog.cpp 1053922 
> 
> Diff: http://reviewboard.kde.org/r/2281/diff
> 
> 
> Testing
> -------
> 
> I have used this dialog box under various combinations of file and/or folder conflicts with all of the buttons.  It appears to work fine.
> 
> 
> Screenshots
> -----------
> 
> Apply to all not pressed
>   http://reviewboard.kde.org/r/2281/s/270/
> Apply to all pressed
>   http://reviewboard.kde.org/r/2281/s/271/
> 
> 
> Thanks,
> 
> Todd
> 
>

More information about the kde-core-devel mailing list