--===============3548608861302938624== Content-Type: multipart/alternative; boundary="===============7550093029772845703==" --===============7550093029772845703== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit ----------------------------------------------------------- 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 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 please: delay initialization of heavy objects such as models until they are actually used. lib/bodega/assetoperations.cpp i don't think that was intentional.. lib/bodega/forumjob_p.cpp 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 should just be called ForumModel lib/bodega/forumjobmodel_p.h 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 is anything going to use this countChanged signal? lib/bodega/forumjobmodel_p.cpp 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 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 > > --===============7550093029772845703== Content-Type: text/html; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit
This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112899/ 

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 (Diff revision 2) 
void AssetOperations::Private::assetDownloadComplete(NetworkJob *job)
 

    82
    
        forumJobModel->setAssetInfo(assetInfo);

  


 





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
    

     (Diff revision 2)

    
   
  

 

 
  


   
void AssetOperations::Private::progressHasChanged(qreal prog)


  

 



 
 

 

  

    
    

    130
    
    d->forumJobModel->setSession(session);

  


 





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














 
  

   
    lib/bodega/assetoperations.cpp
    

     (Diff revision 2)

    
   
  

 

 
  


   
void AssetOperations::Private::progressHasChanged(qreal prog)


  

 



 
 

 

  

    126
    
    AssetJob *aj = session->asset(assetId, AssetJob::ShowRatings | AssetJob::ShowChangeLog);

    132
    
    AssetJob *aj = session->asset(assetId, AssetJob::ShowChangeLog);

  


 





i don't think that was intentional..














 
  

   
    lib/bodega/forumjob_p.cpp
    

     (Diff revision 2)

    
   
  

 



 
 

 

  

    
    

    68
    
    for (itr = topics.constBegin(); itr != topics.constEnd(); ++itr) {

  


  

    
    

    69
    
        QVariantMap info = itr->toMap();

  


  

    
    

    70
    
        ForumCategory::ForumTopic topic;

  


  

    
    

    71
    
        topic.topicName = info[QLatin1String("fancy_title")].toString();

  


  

    
    

    72
    
        topic.topicId = info[QLatin1String("id")].toString();

  


  

    
    

    73
    
        topic.topicSlug = info[QLatin1String("slug")].toString();

  


  

    
    

    74
    
        m_category.topics.append(topic);

  


  

    
    

    75
    
        QUrl url = QLatin1String("http://localhost:3000/t/") + topic.topicSlug + QLatin1String("/") + topic.topicId + QLatin1String(".json"); //FIXME

  


  

    
    

    76
    
        qDebug() << url;

  


  

    
    

    77
    
        ForumTopicJob *topicJob = new ForumTopicJob(get(url), m_session);

  


  

    
    

    78
    

  


  

    
    

    79
    
        connect(topicJob, SIGNAL(jobFinished(Bodega::NetworkJob *)),

  


  

    
    

    80
    
            this, SLOT(topicFinished(Bodega::NetworkJob *)));

  


  

    
    

    81
    
    }

  


 





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
    

     (Diff revision 2)

    
   
  

 



 
 

 

  

    
    

    31
    
    class ForumJobModel : public QAbstractItemModel

  


 





should just be called ForumModel














 
  

   
    lib/bodega/forumjobmodel_p.h
    

     (Diff revision 2)

    
   
  

 



 
 

 

  

    
    

    39
    
            AssetIdRole = Qt::UserRole + 100,

  


  

    
    

    40
    
            AssetLicenseRole = Qt::UserRole + 101,

  


  

    
    

    41
    
            AssetPartnerIdRole = Qt::UserRole + 102,

  


  

    
    

    42
    
            AssetPartnerNameRole = Qt::UserRole + 103,

  


  

    
    

    43
    
            AssetNameRole = Qt::UserRole + 104,

  


  

    
    

    44
    
            AssetVersionRole = Qt::UserRole + 105,

  


  

    
    

    45
    
            AssetFilenameRole = Qt::UserRole + 106,

  


  

    
    

    46
    
            AssetDescriptionRole = Qt::UserRole + 107,

  


  

    
    

    47
    
            AssetPointsRole = Qt::UserRole + 108,

  


 





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
    

     (Diff revision 2)

    
   
  

 



 
 

 

  

    
    

    49
    
    connect(this, SIGNAL(rowsInserted(QModelIndex,int,int)),

  


  

    
    

    50
    
            this, SIGNAL(countChanged()));

  


  

    
    

    51
    
    connect(this, SIGNAL(rowsRemoved(QModelIndex,int,int)),

  


  

    
    

    52
    
            this, SIGNAL(countChanged()));

  


  

    
    

    53
    
    connect(this, SIGNAL(modelReset()),

  


  

    
    

    54
    
            this, SIGNAL(countChanged()));

  


 





is anything going to use this countChanged signal?














 
  

   
    lib/bodega/forumjobmodel_p.cpp
    

     (Diff revision 2)

    
   
  

 



 
 

 

  

    
    

    55
    
    connect(this, SIGNAL(assetInfoChanged()),

  


  

    
    

    56
    
            this, SLOT(fetchCategory()));

  


 





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
    

     (Diff revision 2)

    
   
  

 



 
 

 

  

    
    

    228
    
            endInsertRows();

  


 





this is mispaired with a beginResetModel in fetchCategory.

worse, if the job fails, nothing gets called. the model will simply "hang".









- Aaron J.






On September 23rd, 2013, 3:47 p.m. UTC, Giorgos Tsiapaliokas wrote:











 

  


Review request for Bodega.


By Giorgos Tsiapaliokas.




Updated Sept. 23, 2013, 3:47 p.m.








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.


  		
 






Testing 



 

  
   
It builds.


  		
 








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)
    • 





View Diff








  		
 











  

 



--===============7550093029772845703==--

--===============3548608861302938624==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
Active mailing list
Active@kde.org
https://mail.kde.org/mailman/listinfo/active

--===============3548608861302938624==--