Unity 8

Merge lp:~stolowski/unity8/results-nullptr-fix into lp:unity8

results-nullptr-fix
Merge into trunk

Proposed by Paweł Stołowski on 2013-09-23

Status:

Merged

Approved by:

Michal Hruby on 2013-09-24

Approved revision:

339

Merged at revision:

340

Proposed branch:

lp:~stolowski/unity8/results-nullptr-fix

Merge into:

lp:unity8

Diff against target:

30 lines (+12/-1)

1 file modified

plugins/Unity/categories.cpp (+12/-1)

To merge this branch:

bzr merge lp:~stolowski/unity8/results-nullptr-fix

Related bugs:

Bug #1211595: unity8 crashed with SIGSEGV in operator()	High	Fix Released
Bug #1228097: unity8 and home/etc. scope crashes in automation in build 57	Critical	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2013-09-24
Michal Hruby (community)		2013-09-23	Approve on 2013-09-24
Review via email: mp+187052@code.launchpad.net

Commit message

Check results model ptr returned by GetResultsFromCategory method from UnityCore.

Description of the change

Check results model ptr returned by GetResultsFromCategory method from UnityCore against nullptr, as it may be null in some cases (namely before channel is opened). If this happens, don't use it to set DeeListModel and just use an empty DeeListModel; it will be set properly when both categories and results models are set on channel opening in UnityCore.

lp:~stolowski/unity8/results-nullptr-fix updated on 2013-09-23

337. By Paweł Stołowski on 2013-09-23: Fixed type in variable name.

Revision history for this message

Michal Hruby (mhr3) wrote on 2013-09-23:

17 + if (unity_results) {
18 + results->setModel(unity_results->model());
19 + } else {

The else branch should call setModel(NULL), although I'm not sure what to do about the cached value in that case.

49 + m_resultshangedConnection.disconnect();
50 + m_resultshangedConnection = m_unityScope->results()->model.changed.connect(sigc::mem_fun(this, &Categories::onScopeResultsModelChanged));

I specifically didn't want that signal, cause now we end up setting the model multiple times.

Revision history for this message

Paweł Stołowski (stolowski) wrote on 2013-09-23:

> 17 + if (unity_results) {
> 18 + results->setModel(unity_results->model());
> 19 + } else {
>
> The else branch should call setModel(NULL), although I'm not sure what to do
> about the cached value in that case.
Why explicit setModel(NULL)? We are dealing with new CategoryResults object here, the model is NULL. As for cached results, I expect we will be notifed by signal, and onCategoriesModelChanged will reset all categories?

>
> 49 + m_resultshangedConnection.disconnect();
> 50 + m_resultshangedConnection =
> m_unityScope->results()->model.changed.connect(sigc::mem_fun(this,
> &Categories::onScopeResultsModelChanged));
>
> I specifically didn't want that signal, cause now we end up setting the model
> multiple times
Right. I think it's safe to assume (after looking into UnityCore - ScopeProxy.cpp) that categories changed signal is enough, since both category and results models are set under the same circumstances - on channel opening. Will remove it.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-09-23:

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity8-ci/1069/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-09-23:

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity8-ci/1073/rebuild

review: Approve (continuous-integration)

lp:~stolowski/unity8/results-nullptr-fix updated on 2013-09-24

338. By Paweł Stołowski on 2013-09-24: Don't connect to results model change signal.

Revision history for this message

Michal Hruby (mhr3) wrote on 2013-09-24:

36 +void Categories::onScopeResultsModelChanged(unity::glib::Object<DeeModel> /* model */)
37 +{
38 + // if scope results model changes, we need to reset all category models
39 + onCategoriesModelChanged(m_unityScope->categories()->model());
40 +}

If it's no longer connected shouldn't it go away?

53 + void onScopeResultsModelChanged(unity::glib::Object<DeeModel> model);

ditto

Why explicit setModel(NULL)? We are dealing with new CategoryResults object here, the model is NULL. As for cached results, I expect we will be notifed by signal, and onCategoriesModelChanged will reset all categories?

If setModel(NULL) is not called, the UI will keep displaying the old model (do we want that?), if it is, it will behave the same way as if an empty model was present (but won't react to searches).

lp:~stolowski/unity8/results-nullptr-fix updated on 2013-09-24

339. By Paweł Stołowski on 2013-09-24: Removed unused signal handler.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-09-24:

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity8-ci/1086/rebuild

review: Approve (continuous-integration)

Revision history for this message

Michal Hruby (mhr3) wrote on 2013-09-24:

Ah, ok the results instance is created just before the start of the diff.. so yea, no need for extra setModel(NULL);

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-09-24:

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity8-ci/1087/rebuild

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Mohammad Rakibul Hasan

Paweł Stołowski

Unity Team

 === modified file 'plugins/Unity/categories.cpp'
 --- plugins/Unity/categories.cpp	2013-09-02 11:21:46 +0000
 +++ plugins/Unity/categories.cpp	2013-09-24 09:29:45 +0000
@@ -21,6 +21,7 @@
  // self
  #include "categories.h"
  #include "categoryresults.h"
++#include <QDebug>
  // TODO: use something from libunity once it's public
  enum CategoryColumn {
@@ -62,7 +63,17 @@
          unsigned categoryIndex = static_cast<unsigned>(index);
          auto unity_results = m_unityScope->GetResultsForCategory(categoryIndex);
--        results->setModel(unity_results->model());
++        if (unity_results) {
++            results->setModel(unity_results->model());
++        } else {
++            // No results model returned by unity core; this can be the case when the global
++            // results model of this scope is still not set in unity core. Don't set backend
++            // model in DeeListModel - it will still beahve properly as an empty model. Since
++            // we're connected to the category model change signal, and it is set by unity core
++            // at the same time as results model (on channel opening), we'll reset category
++            // results models with proper models when we're notifed again.
++            qWarning() << "No results model for category" << categoryIndex;
++        }
          m_results.insert(index, results);
+     }