[Konsole-devel] Review Request 110318: Improvements in ColorSchemeEditor dialog

[Kurt Hindenburg]

> On May 6, 2013, 2:48 p.m., Kurt Hindenburg wrote:
> > src/ColorSchemeEditor.cpp, line 243
> > <http://git.reviewboard.kde.org/r/110318/diff/1/?file=142269#file142269line243>
> >
> >     This will force the table to be vertical enough to show all the colors.  I don't see a reason to do this.
> 
> renan fargetton wrote:
>     Since intense colors are in a third column with this patch, this table is half the height it used to be. Even with the fixed height this dialog is still smaller than its parent (EditProfileDialog) minumum height, so I don't think it is really an issue. With my settings colorTable is ~320px height, and the whole dialog ~500px.
>     
>     The alternative would be to let it with Prefered sizePolicy and resize it to the good default size. I tried it first but I didn't get it to work well : since the table is inside a layout, it looks like it is the parent dialog that must be resized, making it complicate to calculate the right height. There is probably a good way to do this, but while I was into the documentation, I thought it was not really worth it, event considering usability : the widget is small enough and the scrolling bar is just making the interface more complicated for no real benefit.
>     
>

Yes I see - since the EditDialog is better the size of the ColorEdit shouldn't matter.


On May 6, 2013, 2:48 p.m., renan fargetton wrote:
> > Thanks for the code and testing.  I think it would be best to split this up into smaller patches.  One for the table fixes and another for the save/apply.  There are also a few minor style issues but that's no big deal.  I'll see if I can get this in before the freeze this month.
> 
> renan fargetton wrote:
>     I have an intermediary commit with the table fix, so it easy to make two patches. Should I make two other review request ? (This is my first patch to kde so I don't know the workflow)
>     
>     I would be happy to learn what are the style issues are since I'm learning. In order to avoid them next time...

I have a patch for the dialog/table ready to go in so you can look at it when I commit it.  If you want to redo the rest of this it would be appreciated. We still have a few weeks until the soft cutoff
- http://techbase.kde.org/Schedules/KDE4/4.11_Release_Schedule

In general, no tabs, watch spaces around "()," and try to keep lines < 80 (really minor) - we try to follow the kdelibs sytle - http://techbase.kde.org/Policies/Kdelibs_Coding_Style


- Kurt


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


On May 5, 2013, 3:45 p.m., renan fargetton wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110318/
> -----------------------------------------------------------
> 
> (Updated May 5, 2013, 3:45 p.m.)
> 
> 
> Review request for Konsole.
> 
> 
> Description
> -------
> 
> Improvements in ColorSchemeEditor dialog :
> - colorTable has one more row for intense colors. This way each color and the corresponding intense color is side by side (the data model colorScheme is unchanged, only the ui has the additional column)
> 
> - colorTable has fixed height such that the table fit exactly inside.
> 
> - an Apply button is added, so that it is possible to save modified ColorScheme directly from colorSchemeEditor dialog. This way it is easier to preview the result of the color theme change, no need to close/open the dialog to get the preview. 
>        * Most of the changes are in EditProfileDialog.cpp where the Kdialog is set up (EditProfileDialog::showColorSchemeEditor() ).
>        * Saving is done in a separated slot EditProfileDialog::saveColorScheme(const ColorScheme& scheme)
> 
> 
> Diffs
> -----
> 
>   src/ColorSchemeEditor.h 9b1e4b2 
>   src/ColorSchemeEditor.cpp e75cacb 
>   src/EditProfileDialog.h 33b3930 
>   src/EditProfileDialog.cpp 74b0237 
> 
> Diff: http://git.reviewboard.kde.org/r/110318/diff/
> 
> 
> Testing
> -------
> 
> Run it on my machine / create and modify colorSchemes with the dialog
> 
> 
> Thanks,
> 
> renan fargetton
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/konsole-devel/attachments/20130507/c4416b0f/attachment.html>