Review Request: Amazon store WIP patch

[Bart Cerneels]

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



src/services/amazon/AmazonCollection.h
<http://git.reviewboard.kde.org/r/102596/#comment5740>

    This class seems so thin that I wonder if it's not better use do the service without using Collection and QueryMaker. Please consider doing the population of the view yourself, so it can suit the store idea better then the simple service TreeView.



src/services/amazon/AmazonMeta.cpp
<http://git.reviewboard.kde.org/r/102596/#comment5734>

    Should not be in the {} block.



src/services/amazon/AmazonMeta.cpp
<http://git.reviewboard.kde.org/r/102596/#comment5737>

    I prefer to use capitals, even in function names, for abbreviations. So setASIN() here. But that is just personal preference that improves readability for me.



src/services/amazon/AmazonParser.h
<http://git.reviewboard.kde.org/r/102596/#comment5738>

    How big is the responce-document going to be? Should you not rather use QXmlStreamReader rather then keeping the entire DOM in memory and having to wait for end of fetching before parsing.



src/services/magnatune/MagnatuneSettingsModule.cpp
<http://git.reviewboard.kde.org/r/102596/#comment5739>

    You probably should commit these fixes to master separately so they don't pollute the already huge patch.


Have you given any thought to a more advanced store browsing UI? In the narrow browser column you probably can only do search results, perhaps also recommendations. People expect a better experience from a music store though.
And like I said, consider doing your own UI for the browser-column (i.e. not ServiceCollection).

- Bart


On Sept. 13, 2011, 12:59 a.m., Sven Krohlas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102596/
> -----------------------------------------------------------
> 
> (Updated Sept. 13, 2011, 12:59 a.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Summary
> -------
> 
> Totally WIP! Many, many TODOs and unfinished stuff in the code... pls ignore. ;-)
> 
> I'm currently stuck at the point that my parser created meta items do not show up in the browser... any idea why?
> 
> 
> Diffs
> -----
> 
>   src/browsers/CollectionTreeItemModelBase.cpp 119f90d 
>   src/core-impl/collections/support/MemoryQueryMakerInternal.h d3825ca 
>   src/core/collections/Collection.h 02b0894 
>   src/core/collections/QueryMaker.h dfca79f 
>   src/services/CMakeLists.txt ad14fda 
>   src/services/ServiceCollection.h 7a57aa6 
>   src/services/ServiceMetaBase.cpp 21587d3 
>   src/services/amazon/Amazon.h PRE-CREATION 
>   src/services/amazon/AmazonActions.h PRE-CREATION 
>   src/services/amazon/AmazonActions.cpp PRE-CREATION 
>   src/services/amazon/AmazonCart.h PRE-CREATION 
>   src/services/amazon/AmazonCart.cpp PRE-CREATION 
>   src/services/amazon/AmazonCollection.h PRE-CREATION 
>   src/services/amazon/AmazonCollection.cpp PRE-CREATION 
>   src/services/amazon/AmazonConfig.h PRE-CREATION 
>   src/services/amazon/AmazonConfig.cpp PRE-CREATION 
>   src/services/amazon/AmazonConfigWidget.ui PRE-CREATION 
>   src/services/amazon/AmazonMeta.h PRE-CREATION 
>   src/services/amazon/AmazonMeta.cpp PRE-CREATION 
>   src/services/amazon/AmazonParser.h PRE-CREATION 
>   src/services/amazon/AmazonParser.cpp PRE-CREATION 
>   src/services/amazon/AmazonSettingsModule.h PRE-CREATION 
>   src/services/amazon/AmazonSettingsModule.cpp PRE-CREATION 
>   src/services/amazon/AmazonStore.h PRE-CREATION 
>   src/services/amazon/AmazonStore.cpp PRE-CREATION 
>   src/services/amazon/CMakeLists.txt PRE-CREATION 
>   src/services/amazon/amarok_service_amazonstore_config.desktop PRE-CREATION 
>   src/services/magnatune/MagnatuneSettingsModule.cpp 15cc57d 
>   src/widgets/SearchWidget.h df16556 
>   src/widgets/SearchWidget.cpp 4b09285 
> 
> Diff: http://git.reviewboard.kde.org/r/102596/diff
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> Unknown entry/entries after parsing
>   http://git.reviewboard.kde.org/r/102596/s/256/
> 
> 
> Thanks,
> 
> Sven
> 
>

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