Review Request: Minor GUI change to rename/overwrite dialog window

Steven Sroka thedude_10160 at hotmail.com
Mon Oct 4 19:37:32 BST 2010 


> On 2010-10-04 17:17:11, Aaron Seigo wrote:
> > i agree with David Jarvie that removing the information about the file being newer/older/etc from the top label is a step backwards: it removes useful information, instead requiring the user to figure it out on their own using displayed dates. i do agree that it makes sense to move the potentially loooong file names down to the representations themselves, however, under the 'source' and 'destination' labels as that makes it clearer which is which and the informational text at the top easier to parse.
> > 
> > however (looking at the screenshots only, haven't rebuilt with the patch to try it first hand), it now has textual information both above and below the preview icon and the new information above (besides not using KLocale, see inline comments below) is visually formatted differently from the rest of the informational text at the bottom. this really stands out because the text below is formatted so nicely. since the modification time information should already be below the preview icon, i don't see the point of duplicating it above as well. perhaps if it was just the file name it would look fine; i'd leave the metadata to the to the UI that is already providing it below the preview.
> > 
> > (btw, something that would be slick is to bind the value of the vertical scrollbars together, or perhaps even better just provide one vertical scrollbar that controls both file views simultaneously. right now to compare metadata, i have to scroll both bars which is a lot of back-and-forth mousing; i'm not sure there is any real use case for "look at the number of words in one text file, while viewing the content of the other" in this dialog, since it is really all about comparing the two files to decide what to do about overwriting the file.)

"haven't rebuilt with the patch to try it first hand"
Unfortunately, you can't just yet. I had to add a new function, which meant that I had to update the header file as well (renamedialog.h). I'm not sure how I can upload the new header file as a diff to this same review request.

"i'd leave the metadata to the to the UI that is already providing it below the preview."
I did not notice the duplication, and the QLabel's I added originally with this duplication have been removed for the next revision.

"provide one vertical scrollbar that controls both file views simultaneously"
I completely agree, but I'm not sure how to do this. Is it possible to have one container with two images side-by-side within it?


> On 2010-10-04 17:17:11, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/kio/kio/renamedialog.cpp, line 271
> > <http://svn.reviewboard.kde.org/r/5443/diff/4/?file=38848#file38848line271>
> >
> >     use spacers instead of empty QLabels?

Whats the name of the class for the spacers? Under what library? I would like to use them as well since QLabels are only workarounds for this kind of situation.


> On 2010-10-04 17:17:11, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/kio/kio/renamedialog.cpp, lines 258-261
> > <http://svn.reviewboard.kde.org/r/5443/diff/4/?file=38848#file38848line258>
> >
> >     this should be done using KLocale, not hand crafted strings.

Good idea, but since I remove the duplication of information, all of this has been deleted and cleaned up. I love cutting code :)


> On 2010-10-04 17:17:11, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/kio/kio/renamedialog.cpp, lines 305-307
> > <http://svn.reviewboard.kde.org/r/5443/diff/4/?file=38848#file38848line305>
> >
> >     why is this squeezed? it's one word and a widget label, it should just be a normal QLabel, and it should be on the same line as the line edit so it is evident what it is referring to.
> >     
> >     (a stylistic comment: "sentence2" is a bit of an indirect name for such a thing; why not just put the i18n call right into the consructor: new QLabel(i18n("Rename"), this);?)

I think I have cleaned up most of the simpler situations where code should have been combined. This was missed though. The KSqueezedTextLabel was changed to QLabel as well.


> On 2010-10-04 17:17:11, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/kio/kio/renamedialog.cpp, line 628
> > <http://svn.reviewboard.kde.org/r/5443/diff/4/?file=38848#file38848line628>
> >
> >     personally, i'd think it would make more sense just add a boolean to the signature of this method instead of comparing strings, which is easy to break in future.
> >     
> >     this, and the rest of the private methods, should be moved to RenameDialogPrivate, as well.

string comparison has been changed to boolean comparison, but I'm not sure how to implement the last suggestion though.


> On 2010-10-04 17:17:11, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/kio/kio/renamedialog.cpp, lines 302-303
> > <http://svn.reviewboard.kde.org/r/5443/diff/4/?file=38848#file38848line302>
> >
> >     why an empty label?

It needs to be a spacer instead to separate the the previews and the rename textbox. This does create blank space, so it not completely necessary. See the *New screenshots to see how it looks.


> On 2010-10-04 17:17:11, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/kio/kio/renamedialog.cpp, lines 242-244
> > <http://svn.reviewboard.kde.org/r/5443/diff/4/?file=38848#file38848line242>
> >
> >     probably should not be bold; bold text tends to have the effect of making everything "heavier" visually without actually improving actual readability when applied to whole sentences like this.

I prefer bold myself so the user can't ignore the message. But I understand what you are getting at. I changed it so you can see how it looks like when I upload a screenshot.


- Steven


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


On 2010-10-03 01:38:23, Steven Sroka wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5443/
> -----------------------------------------------------------
> 
> (Updated 2010-10-03 01:38:23)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> This is my first submission to KDE. If I am missing something, don't hesitate to tell me.
> 
> This is a slight GUI change to the rename/overwrite dialog window, just to make it more user friendly.
> 
> 
> This addresses bug 238942.
>     https://bugs.kde.org/show_bug.cgi?id=238942
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/kio/kio/renamedialog.cpp 1181964 
> 
> Diff: http://svn.reviewboard.kde.org/r/5443/diff
> 
> 
> Testing
> -------
> 
> Correctly Compiled.
> "Eye ball'ed" Code.
> Ran code with all possible '_mode' as per http://api.kde.org/4.5-api/kdelibs-apidocs/kio/html/namespaceKIO.html#bac5df6792cd3426582dbfd1af706bff
> Ran many possible combinations (most if not all) -> move folder to folder, file to file, file to folder, folder to file, and paid attention to creation date.
> 
> (I actually found a bug with the preview picture that is shown in a certain scenario - I will create a bug notice for it on bugs.kde.org soon)
> 
> 
> Screenshots
> -----------
> 
> KDE Trunk - Folder to Folder
>   http://svn.reviewboard.kde.org/r/5443/s/524/
> *New - Folder to Folder
>   http://svn.reviewboard.kde.org/r/5443/s/525/
> KDE Trunk - Bug
>   http://svn.reviewboard.kde.org/r/5443/s/526/
> *New - File to File
>   http://svn.reviewboard.kde.org/r/5443/s/527/
> KDE 4.5 - File to File
>   http://svn.reviewboard.kde.org/r/5443/s/528/
> 
> 
> Thanks,
> 
> Steven
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20101004/7f2e0dce/attachment.htm>

More information about the kde-core-devel mailing list