Review Request: Layout changes to organize collection, guess metadata, edit filter and edit playlist layout dialogs

[Matěj Laitl]

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


Okay, I've found some time to review the code, it contains many overdue clean-ups, good work, Ralf! Unambiguously positive changes. Some smaller remarks and merging consideration:
 * [minor] Please strip off newly added DEBUG_BLOCKs, our debugging spam is excessive
 * [minor] I've seen static_cast<EnumType>( int something ); <- constructor-like cast is preferred in this case as it behaves better wrt refactoring: EnumType enum = EnumType( int );

 * the patch contains i18n changes. We're in string freeze, so it cannot be merged as-is. It either needs for wait for 2.8 (fine with me) or the i18n changes would need to be (temporarily) stripped off. (could be a headache) Ralf, what would you prefer?

Ralf, I still prefer reviewing code here on reviewboard, could you please post the combined patch of your branch? `post-review --branch=$(git symbolic-ref HEAD | cut -d/ -f 3) --username=rengels --tracking-branch master --parent=$(git merge-base HEAD master) --open -r 107624` ran from the branch should work.

- Matěj Laitl


On Dec. 7, 2012, 11:44 a.m., Ralf Engels wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107624/
> -----------------------------------------------------------
> 
> (Updated Dec. 7, 2012, 11:44 a.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> Several refactoring changes centering around token pool, token, token drop target and so on.
> 
> 1. splitting up the FilenameLayoutDialog (which was not a dialog at all) into two widgets to be used by the actual dialog.
> 2. changing the token pool from an icon list to a normal list to prevent the use from having to scroll so much.
> 3. simplifying the drop target so that it does not need to install event filters for it's parents.
> 4. fixing small issues in the token pool, token and token drop target so that they have sensible minimumSizeHint and sizeHints
> 5. aligning texts between the different token users. No longer different texts.
> 6. for the edit filter dialog changes to the layout to get rid of the space waste in the result area.
> 7. for the guess tag dialog I got rid of some empty areas and some useless settings (settings that the dialog could determine itself)
> 8. for the playlist layout dialog not much has changed except that we don't need the event-filter parent mechanism.
> 
> Have a look at the attached screenshots to see the differences.
> The actual code changes are in the rengels-filenameLayoutDialog branch.
> 
> Further work: Settings and presets need a reworking, as we have several sets currently which is confusing.
> 
> 
> Diffs
> -----
> 
>   ChangeLog 15a698c 
> 
> Diff: http://git.reviewboard.kde.org/r/107624/diff/
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> Old Layout editor dialog
>   http://git.reviewboard.kde.org/r/107624/s/878/
> New Layout editor dialog
>   http://git.reviewboard.kde.org/r/107624/s/879/
> Old Guess Tag Dialog
>   http://git.reviewboard.kde.org/r/107624/s/880/
> New Guess Tag Dialog
>   http://git.reviewboard.kde.org/r/107624/s/881/
> Old Edit Filter Dialog
>   http://git.reviewboard.kde.org/r/107624/s/882/
> New Edit Filter Dialog
>   http://git.reviewboard.kde.org/r/107624/s/883/
> 
> 
> Thanks,
> 
> Ralf Engels
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20121209/a3504c08/attachment.html>