[prev in list] [next in list] [prev in thread] [next in thread]
List: amarok-devel
Subject: Re: Review Request: Fix problem with inline editing in playlist.
From: Thomas_Lübking <thomas.luebking () web ! de>
Date: 2010-10-18 16:24:28
Message-ID: 20101018162428.26629.21101 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On 2010-10-15 07:46:32, Mark Kretschmann wrote:
> > Ralf, with my "Verbose" playlist layout, all the lines have become about 50% \
> > taller with your patch. So the playlist takes up much more vertical space.
> > Is this intended? I'm not sure it's a good idea to use so much vertical space.
> >
>
> Ralf Engels wrote:
> The reason for the fix was that when editing an entry in the collection view the \
> space was not enough to display the line edits and rating widget completely. This \
> starts with a two lined layout where the second line edit is missing the two lowest \
> pixels. It get's worse when editing a compressed (as in two songs from the same \
> album) entry where the line edits overlap the header. It is completely unusable if \
> you have a third line with only a rating widget.
> Only three solutions come to my mind:
> 1. allow the edit components to overlap.
> This is not really nice as we would overlap uncontrolled e.g. at the end of the \
> list. 2. change the layout of the item when going into edit mode.
> I don't really know how to do this nicely.
> 3. Calculate the height of the items so that the edit widgets will have their \
> minimum size. That is what I am doing.
> I am even cheating a little because in the "normal" mode I allow a margin around \
> the item which is not there in the edit mode. Also now the list has the same layout \
> as all the other lists, which you can see when comparing the item height against \
> the item height of e.g. the dynamic playlist items.
> I would rather have a little bit larger list that supports three row layouts.
>
> Nikolaj Hald Nielsen wrote:
> How about simply changing the style of the edit boxes to make them the same height \
> as a line of text? You can do some pretty cool stuff with stylesheets.
> I dont think that making each line in the playlist taller for all users in _all_ \
> cases, even the ones that never use the inline editing is acceptable.
w/o having looked at the code you likely want
QLineEdit::setFrame(false);
this should shrink the size and if everything else fails you could still
QWidget::setFixedHeight(h);
So there's likely no need to hardcode stylesheets - which can easily cause trouble by \
hardcoding colors :-(
Otherwise i'm with Nikolaj - styles could define *large* paddings for lineedits (eg. \
for a "match button height" policy or so) and i may recall that there've been various \
mourns that "the new $%&/()= playlist wastes too much space" =P
... ok and actually looking at the code:
the proper function for styled lineedit heights is \
"QStyle::sizeFromContents(CT_LineEdit, .*)" and not some pixelmetrics + whatever \
oxygen randomly adds to it, thanks alot ;-)
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100039/#review90
-----------------------------------------------------------
On 2010-10-09 18:27:00, Ralf Engels wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100039/
> -----------------------------------------------------------
>
> (Updated 2010-10-09 18:27:00)
>
>
> Review request for Amarok.
>
>
> Summary
> -------
>
> Fixes three problems:
> 1. Alignment was not considered with rating widget when editing
> 2. Calculation of item height was incorrect leading to the problem that three line \
> items would not show the third line when editing 3. QStyle pixel metrics were \
> disregarded
>
> Diffs
> -----
>
> src/playlist/view/listview/InlineEditorWidget.h f14d6df
> src/playlist/view/listview/InlineEditorWidget.cpp a7e981b
> src/playlist/view/listview/PrettyItemDelegate.h a929ecb
> src/playlist/view/listview/PrettyItemDelegate.cpp 31d4379
> src/playlist/view/listview/PrettyListView.cpp ca3d580
>
> Diff: http://git.reviewboard.kde.org/r/100039/diff
>
>
> Testing
> -------
>
> Editing with one, two and three line layouts.
> Using Oxygen and normal styles.
>
>
> Thanks,
>
> Ralf
>
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/100039/">http://git.reviewboard.kde.org/r/100039/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On October 15th, 2010, 7:46 a.m., <b>Mark \
Kretschmann</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px \
solid #d0d0d0; padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Ralf, with my "Verbose" playlist layout, all the lines have \
become about 50% taller with your patch. So the playlist takes up much more vertical \
space.
Is this intended? I'm not sure it's a good idea to use so much vertical \
space. </pre>
</blockquote>
<p>On October 18th, 2010, 10:02 a.m., <b>Ralf Engels</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The reason for the fix \
was that when editing an entry in the collection view the space was not enough to \
display the line edits and rating widget completely. This starts with a two lined \
layout where the second line edit is missing the two lowest pixels. It get's \
worse when editing a compressed (as in two songs from the same album) entry where the \
line edits overlap the header. It is completely unusable if you have a third line \
with only a rating widget.
Only three solutions come to my mind:
1. allow the edit components to overlap.
This is not really nice as we would overlap uncontrolled e.g. at the end of the list.
2. change the layout of the item when going into edit mode.
I don't really know how to do this nicely.
3. Calculate the height of the items so that the edit widgets will have their minimum \
size. That is what I am doing.
I am even cheating a little because in the "normal" mode I allow a margin \
around the item which is not there in the edit mode. Also now the list has the same \
layout as all the other lists, which you can see when comparing the item height \
against the item height of e.g. the dynamic playlist items.
I would rather have a little bit larger list that supports three row layouts.</pre>
</blockquote>
<p>On October 18th, 2010, 11:33 a.m., <b>Nikolaj Hald Nielsen</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">How about simply \
changing the style of the edit boxes to make them the same height as a line of text? \
You can do some pretty cool stuff with stylesheets.
I dont think that making each line in the playlist taller for all users in _all_ \
cases, even the ones that never use the inline editing is acceptable. </pre> \
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">w/o having looked at the \
code you likely want QLineEdit::setFrame(false);
this should shrink the size and if everything else fails you could still
QWidget::setFixedHeight(h);
So there's likely no need to hardcode stylesheets - which can easily cause \
trouble by hardcoding colors :-(
Otherwise i'm with Nikolaj - styles could define *large* paddings for lineedits \
(eg. for a "match button height" policy or so) and i may recall that \
there've been various mourns that "the new $%&/()= playlist wastes too \
much space" =P
... ok and actually looking at the code:
the proper function for styled lineedit heights is \
"QStyle::sizeFromContents(CT_LineEdit, .*)" and not some pixelmetrics + \
whatever oxygen randomly adds to it, thanks alot ;-)</pre> <br />
<p>- Thomas</p>
<br />
<p>On October 9th, 2010, 6:27 p.m., Ralf Engels wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for Amarok.</div>
<div>By Ralf Engels.</div>
<p style="color: grey;"><i>Updated 2010-10-09 18:27:00</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Fixes three problems: 1. Alignment was not considered with rating widget \
when editing 2. Calculation of item height was incorrect leading to the problem that \
three line items would not show the third line when editing 3. QStyle pixel metrics \
were disregarded </pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Editing with one, two and three line layouts. Using Oxygen and normal \
styles.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/playlist/view/listview/InlineEditorWidget.h <span style="color: \
grey">(f14d6df)</span></li>
<li>src/playlist/view/listview/InlineEditorWidget.cpp <span style="color: \
grey">(a7e981b)</span></li>
<li>src/playlist/view/listview/PrettyItemDelegate.h <span style="color: \
grey">(a929ecb)</span></li>
<li>src/playlist/view/listview/PrettyItemDelegate.cpp <span style="color: \
grey">(31d4379)</span></li>
<li>src/playlist/view/listview/PrettyListView.cpp <span style="color: \
grey">(ca3d580)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/100039/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic