Review Request 112899: add support for the forum in bodega-client(1 of 2)

[Aaron J. Seigo]

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


This patch is incorrect from start to finish, I'm afraid. As has been noted several times now, the fact that bodega is using discourse as the forum software must be encapsulated inside of the bodega server. The only place this can "leak" is in the forum URL to be  handed to a full web browser for when the user wishes to interact fully with the forums.

However, *all* access to the forums from a bodega client *must* be done via bodega server API. It must *never* touch discourse directly as that leads to several problems:

* we can never switch away from discourse easily, should we decide that is necessary 
* we  end up with exactly the sort of problems seen in ForumCategoryJob where instead of getting a self-contained response it has to do multiple trips, not use the Session object and do hacks around the jobFinished signal as a result

To summarize:

* there needs to be API in bodega-server that allows one to fetch postings from the forum related to an asset; this API needs to support paging so that only the first few responses can be fetched and then more on demand
* the client-side code needs to use this server-side API rather than discourse directly

I am very certain we have had this exact discussion before.


lib/bodega/assetoperations.cpp
<http://git.reviewboard.kde.org/r/112899/#comment29937>

    so as soon as the asset info is retrieved, the forumJobModel, even if nothing is showing the forum responses, is immediately sent off to fetch all the responses?
    
    unlike the ratings model which does make sense as part of AssetOperations, i don't see why the forum model must be.



lib/bodega/assetoperations.cpp
<http://git.reviewboard.kde.org/r/112899/#comment29935>

    please: delay initialization of heavy objects such as models until they are actually used.



lib/bodega/assetoperations.cpp
<http://git.reviewboard.kde.org/r/112899/#comment29934>

    i don't think that was intentional..



lib/bodega/forumjob_p.cpp
<http://git.reviewboard.kde.org/r/112899/#comment29938>

    it fetches the *entire* set of discussions and feedback? what happens when an asset has 100s of replies and the person is accessing it over e.g. a 3G connection?
    
    as we talked about previously: by default only the most recent responses should be fetched and the rest loaded on demand.



lib/bodega/forumjobmodel_p.h
<http://git.reviewboard.kde.org/r/112899/#comment29933>

    should just be called ForumModel



lib/bodega/forumjobmodel_p.h
<http://git.reviewboard.kde.org/r/112899/#comment29930>

    this  data is not unique per-row, it has nothing to do with the forum feedback, i do not know why it is here.



lib/bodega/forumjobmodel_p.cpp
<http://git.reviewboard.kde.org/r/112899/#comment29932>

    is anything going to use this countChanged signal?



lib/bodega/forumjobmodel_p.cpp
<http://git.reviewboard.kde.org/r/112899/#comment29936>

    why would you use a signal for this? just call it directly from setAssetInfo. you are making the code more complicated that it needs to be.



lib/bodega/forumjobmodel_p.cpp
<http://git.reviewboard.kde.org/r/112899/#comment29931>

    this is mispaired with a beginResetModel in fetchCategory.
    
    worse, if the job fails, nothing gets called. the model will simply "hang".


- Aaron J. Seigo


On Sept. 23, 2013, 3:47 p.m., Giorgos Tsiapaliokas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112899/
> -----------------------------------------------------------
> 
> (Updated Sept. 23, 2013, 3:47 p.m.)
> 
> 
> Review request for Bodega.
> 
> 
> Description
> -------
> 
> This patch adds the forumjob and the forumjobmodel.
> The patch doesn't contain any ui bits in order to avoid
> an endless review request. I will open the second one after this one.
> 
> 
> Diffs
> -----
> 
>   lib/bodega/CMakeLists.txt 6fd0d0b 
>   lib/bodega/assetjob.cpp 3423c41 
>   lib/bodega/assetoperations.h c4ce191 
>   lib/bodega/assetoperations.cpp b1fd346 
>   lib/bodega/forumjob_p.h PRE-CREATION 
>   lib/bodega/forumjob_p.cpp PRE-CREATION 
>   lib/bodega/forumjobmodel_p.h PRE-CREATION 
>   lib/bodega/forumjobmodel_p.cpp PRE-CREATION 
>   lib/bodega/globals.h c696a40 
> 
> Diff: http://git.reviewboard.kde.org/r/112899/diff/
> 
> 
> Testing
> -------
> 
> It builds.
> 
> 
> Thanks,
> 
> Giorgos Tsiapaliokas
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/active/attachments/20130924/b686d9d0/attachment-0001.html>