Review Request: RSS dataengine now supports multiple feeds.

[Aaron Seigo]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://matt.rogers.name/r/111/#review104
-----------------------------------------------------------


I can understand the use case for this, though I have some questions as to the implementation.

Also, while it would be nice to be able to share feeds fully (e.g. "A,C" and "B,C" would share fetching of "C") i can see how that's not a likely to really have real world usefulness and so probably isn't worthwhile chasing down.

Also, the indentation and formatting in the patch is not in line with the rest of the file. We really do try and keep consistency within the code as it prevents it from becoming a difficult to read (and therefore maintain) soup. =)


trunk/playground/base/plasma/engines/rss/rss.cpp
<http://matt.rogers.name/r/111/#comment82>

    this will break if there is a legitimate ',' in the feed URL?



trunk/playground/base/plasma/engines/rss/rss.cpp
<http://matt.rogers.name/r/111/#comment84>

    this probably won't work quite as expected: visualizations will be connected to the source of the name passed into updateSource. so the individual feeds won't be picked up automatically by them, and this may end up clobbering other legitimate requests for these feeds individually.
    
    e.g. if one visualizations requests feed A, and another requests feed "A,B" then feed A will get updated more often than necessary and the second visualization won't get notified of changes to A or B individually at all.
    
    this also implies that all timing constraints will be thrown out the window as well for the individual sources.
    
    i'm not sure what is really gained by having the feeds  individually published when in a combined list?  



trunk/playground/base/plasma/engines/rss/rss.cpp
<http://matt.rogers.name/r/111/#comment83>

    Engines will at worst run in their own thread, but there are no plans to multithread individual engines. So if this came up as an issue, it would be because threading was added to the RssEngine directly.


- Aaron


On 2008-02-11 10:12:38, Rob Scheepmaker wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://matt.rogers.name/r/111/
> -----------------------------------------------------------
> 
> (Updated 2008-02-11 10:12:38)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> My first contribution to the KDE project! This diff should add support for requesting a comma separated list of multiple feeds from the RSS dataengine, which will merge the entries from the different feeds in one convenient time sorted list. I'm really new to QT / plasma, so I've probably done some things wrong... so I'm looking forward to getting feedback from people who actually know what they're doing. ;)
> 
> 
> Diffs
> -----
> 
>   trunk/playground/base/plasma/engines/rss/rss.h
>   trunk/playground/base/plasma/engines/rss/rss.cpp
> 
> Diff: http://matt.rogers.name/r/111/diff
> 
> 
> Testing
> -------
> 
> Not a lot atm. I've tested with both plasmaengineexplorer and the news applet in trunk. Feeding the news applet in trunk with a comma seperated list of url's shows that it works! Data abstraction rocks :). It seems however that the first time the data is requested, the feeds aren't really merged... the next update works fine. I'm still looking into that.
> 
> 
> Thanks,
> 
> Rob
> 
>