Review Request: Allow guessing of tags from filenames for multiple files

[Matěj Laitl]

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


Good work and I agree that we should provide the funcitonality, but some issues need to be addressed first:
 * this patch contains both the feature and cleanups. Split it, create review request for all the cleaups and interface changes (to merged first) and then implement the feature in a commit that would contain _no_ cleanups or not-needed changes.
 * we generally favour Meta::val* over Meta::FIELD::*. In fact, my long-term goal is to ditch Meta::FIELD::* completely.
 * how the changes to the UI look like? How are multiple filenames shown in "Filename Layout Chooser"?


shared/TagsFromFileNameGuesser.cpp
<http://git.reviewboard.kde.org/r/105875/#comment14794>

    This can (and should) be completely replaced by Meta::fieldForName(), solving bug 200516.



shared/TagsFromFileNameGuesser.cpp
<http://git.reviewboard.kde.org/r/105875/#comment14793>

    Please document this method and make it static (.cpp - only).
    
    Also, you now return type of the field, which is inappropriate for this method.
    
    I propose introducing
    QVariant::Type
    Meta::typeForField()
    to MetaConstants.h



shared/TagsFromFileNameGuesser.cpp
<http://git.reviewboard.kde.org/r/105875/#comment14795>

    Please remove trailing whitespace when at it.



shared/TagsFromFileNameGuesser.cpp
<http://git.reviewboard.kde.org/r/105875/#comment14797>

    Good.



shared/TagsFromFileNameGuesser.cpp
<http://git.reviewboard.kde.org/r/105875/#comment14796>

    please remove trailing whitespace



shared/TagsFromFileNameGuesser.cpp
<http://git.reviewboard.kde.org/r/105875/#comment14798>

    Use Meta::typeForField here.



src/core/meta/support/MetaConstants.h
<http://git.reviewboard.kde.org/r/105875/#comment14800>

    Unnecessary, ditch.



src/core/meta/support/MetaConstants.h
<http://git.reviewboard.kde.org/r/105875/#comment14801>

    Leave this specific to TagDialog.cpp, please.



src/core/meta/support/MetaConstants.cpp
<http://git.reviewboard.kde.org/r/105875/#comment14802>

    Unnecessary, ditch.



src/dialogs/FilenameLayoutDialog.cpp
<http://git.reviewboard.kde.org/r/105875/#comment14799>

    Don't use C arrays. Qt!!!
    
    Otherwise welcome cleanup that should've been put into another review request.



src/dialogs/TagDialog.h
<http://git.reviewboard.kde.org/r/105875/#comment14803>

    Please document.



src/dialogs/TagDialog.cpp
<http://git.reviewboard.kde.org/r/105875/#comment14804>

    Hmm, don't mix unrelated cleanups with features, please. Open another review for cleanups.



src/dialogs/TagDialog.cpp
<http://git.reviewboard.kde.org/r/105875/#comment14805>

    Oh noes, C array again. :-)
    
    Otherwise welcome cleanup that should've been put into another review request.



src/dialogs/TagDialog.cpp
<http://git.reviewboard.kde.org/r/105875/#comment14806>

    Ouch? You dereference the pointer and then you make a reference of it? Just use the pointer, no?



src/dialogs/TagGuesser.h
<http://git.reviewboard.kde.org/r/105875/#comment14807>

    This is a change to worse. Change to Meta::FieldHash instad!



src/dialogs/TagGuesser.h
<http://git.reviewboard.kde.org/r/105875/#comment14809>

    Take qint64 field as argument.



src/dialogs/TagGuesser.h
<http://git.reviewboard.kde.org/r/105875/#comment14808>

    Again, Meta::FieldHash.



src/dialogs/TagGuesser.cpp
<http://git.reviewboard.kde.org/r/105875/#comment14810>

    Unused variable now.



src/dialogs/TagGuesser.cpp
<http://git.reviewboard.kde.org/r/105875/#comment14811>

    Welcome cleanup, but use Meta::val* instead.


- Matěj Laitl


On Aug. 6, 2012, 10:14 a.m., Matthias Berndt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105875/
> -----------------------------------------------------------
> 
> (Updated Aug. 6, 2012, 10:14 a.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> Allow guessing of tags from filenames for multiple files
> (bug #213074).
> 
> Refactor some of the code in order to reuse some of the code intended
> for MusicBrainz
> 
> make the tag guessing dialog non-modal
> 
> 
> This addresses bug 213074.
>     https://bugs.kde.org/show_bug.cgi?id=213074
> 
> 
> Diffs
> -----
> 
>   shared/TagsFromFileNameGuesser.cpp f2ec2ba 
>   src/core/meta/support/MetaConstants.h 41c78a9 
>   src/core/meta/support/MetaConstants.cpp 079fd22 
>   src/dialogs/FilenameLayoutDialog.cpp 1517a8a 
>   src/dialogs/TagDialog.h 015f2d3 
>   src/dialogs/TagDialog.cpp a448458 
>   src/dialogs/TagGuesser.h 432d38f 
>   src/dialogs/TagGuesser.cpp b1a503f 
> 
> Diff: http://git.reviewboard.kde.org/r/105875/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Matthias Berndt
> 
>

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