Merge lp://qastaging/~tiagosh/unity-2d/unity-2d-shell-homelens into lp://qastaging/unity-2d

Proposed by Tiago Salem Herrmann
Status: Merged
Approved by: Albert Astals Cid
Approved revision: 952
Merged at revision: 910
Proposed branch: lp://qastaging/~tiagosh/unity-2d/unity-2d-shell-homelens
Merge into: lp://qastaging/unity-2d
Diff against target: 341 lines (+92/-64)
9 files modified
libunity-2d-private/src/lens.cpp (+15/-3)
libunity-2d-private/src/lenses.cpp (+8/-5)
libunity-2d-private/src/lenses.h (+2/-0)
shell/app/shelldeclarativeview.cpp (+7/-0)
shell/app/shelldeclarativeview.h (+2/-0)
shell/dash/Dash.qml (+25/-17)
shell/dash/LensBar.qml (+20/-19)
shell/dash/LensView.qml (+7/-6)
shell/dash/RendererGrid.qml (+6/-14)
To merge this branch: bzr merge lp://qastaging/~tiagosh/unity-2d/unity-2d-shell-homelens
Reviewer Review Type Date Requested Status
Albert Astals Cid (community) Approve
Gerry Boland Pending
Michał Sawicz Pending
Review via email: mp+92571@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2012-01-31.

Description of the change

This MR replaces the old home lens by the new one provided by unity.
The following libqtdee branch must be installed in order to compile this branch: lp:~tiagosh/dee-qt/dee-qt-local-models .

To post a comment you must log in.
Revision history for this message
Gerry Boland (gerboland) wrote : Posted in a previous version of this proposal

Hi Tiago,
great work, thanks for this! But we need tests to be able to accept this.

I think testing the following will be sufficient:
1. open dash, check somehow that this Home lens is showing
2. open dash, type a character, check search results appear in Home lens

Consult the existing tests to get an idea where to start. Getting started guide here:
https://wiki.ubuntu.com/Unity2DTestability
and this old test of mine might help you get started. Not it's for unity-2d, so things have changed a little
https://bazaar.launchpad.net/~gerboland/unity-2d/dash-test/view/head:/tests/places/show_hide_tests.rb

Ping me if you need a hand
-G

review: Needs Fixing
Revision history for this message
Michał Sawicz (saviq) wrote : Posted in a previous version of this proposal

Hey, this also looks like it could go into lp:unity-2d almost without modification. As it's a goal to have as small a diff between trunk and shell when we get to merging it, could you please have a MR against lp:unity-2d for this?

review: Needs Information
Revision history for this message
Michał Sawicz (saviq) wrote : Posted in a previous version of this proposal

As discussed, let's wait with this until shell gets merged into trunk.

review: Abstain
Revision history for this message
Gerry Boland (gerboland) wrote : Posted in a previous version of this proposal

Hey,
this is great work, thank you Tiago. It requires some fixes though:
- first, your patch to dee-qt may need fixing. I'm blaming it for these problems:
    - on first run, all the icons were ? icons with no text.
    - bug I can reproduce:
      1. I hope dash. I search for something, I get correct results
      2. if I empty search box, everything is still good
      3. I switch to applications lens, everything good
      4. I switch back to home lens, I get many ? icons everywhere with no text (just like first run)
  But the bug may be here. I've not tried to dig yet.
  Here's screengrab to show you: https://imgur.com/wTMaN

- possibly as a consequence, I get these unity-2d errors when viewing Home lens
[WARNING] shell/dash/RendererGrid.qml:92: ReferenceError: Can't find variable: column_0
[WARNING] shell/dash/RendererGrid.qml:92: ReferenceError: Can't find variable: display

[WARNING] shell/dash/LensView.qml:87: Unable to assign [undefined] to QString iconHint
[WARNING] shell/dash/LensView.qml:88: Unable to assign [undefined] to QString rendererName
[WARNING] shell/dash/LensView.qml:86: Unable to assign [undefined] to QString name
   <repeated many times>

[WARNING] shell/dash/RendererGrid.qml:92: ReferenceError: Can't find variable: column_0
   <repeated many times>

But your code looks clean, nothing obvious appears wrong to me.

review: Needs Fixing
Revision history for this message
Albert Astals Cid (aacid) wrote : Posted in a previous version of this proposal

Doesn't merge cleanly with lp:~unity-2d-team/unity-2d/unity-2d-shell Can you remerge it?

review: Needs Fixing
Revision history for this message
Albert Astals Cid (aacid) : Posted in a previous version of this proposal
review: Approve (fun)
Revision history for this message
Albert Astals Cid (aacid) wrote : Posted in a previous version of this proposal

With fun i mean "Functional" :/

I still have to look at the code, there is also
unity-2d-shell: [CRITICAL] dee: dee_model_get_first_iter: assertion `DEE_IS_MODEL (self)' failed
unity-2d-shell: [CRITICAL] dee: dee_model_get_last_iter: assertion `DEE_IS_MODEL (self)' failed
unity-2d-shell: [CRITICAL] dee: dee_model_get_first_iter: assertion `DEE_IS_MODEL (self)' failed
unity-2d-shell: [CRITICAL] dee: dee_model_get_last_iter: assertion `DEE_IS_MODEL (self)' failed
unity-2d-shell: [CRITICAL] dee: dee_model_get_first_iter: assertion `DEE_IS_MODEL (self)' failed
unity-2d-shell: [CRITICAL] dee: dee_model_get_last_iter: assertion `DEE_IS_MODEL (self)' failed
unity-2d-shell: [CRITICAL] dee: dee_model_get_first_iter: assertion `DEE_IS_MODEL (self)' failed
unity-2d-shell: [CRITICAL] dee: dee_model_get_last_iter: assertion `DEE_IS_MODEL (self)' failed

That thiago says that can't remove but are somehow scary for my pov

Revision history for this message
Albert Astals Cid (aacid) wrote : Posted in a previous version of this proposal

And with Thiago i mean Tiago

Revision history for this message
Florian Boucault (fboucault) wrote : Posted in a previous version of this proposal

Please resubmit the MR against lp:unity-2d

Revision history for this message
Albert Astals Cid (aacid) wrote :

Had a more thorough look at those warnings and they really seem to come from unity libraries themselves without much interaction in our side.

So Approve it is

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches