gallery-app

Merge lp:~artmello/gallery-app/gallery-app-fix_toolbar_device into lp:gallery-app

gallery-app-fix_toolbar_device
Merge into trunk

Proposed by Arthur Mello on 2014-06-21

Status:

Merged

Approved by:

Olivier Tilloy on 2014-06-25

Approved revision:

1005

Merged at revision:

1002

Proposed branch:

lp:~artmello/gallery-app/gallery-app-fix_toolbar_device

Merge into:

lp:gallery-app

Diff against target:

55 lines (+10/-6)

2 files modified

src/gallery-application.cpp (+7/-4)
src/gallery-manager.cpp (+3/-2)

To merge this branch:

bzr merge lp:~artmello/gallery-app/gallery-app-fix_toolbar_device

Critical

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Olivier Tilloy			Approve on 2014-06-25
PS Jenkins bot	continuous-integration		Needs Fixing on 2014-06-25
Gustavo Pichorim Boiko (community)			Approve on 2014-06-24
Oliver Grawert		2014-06-21	Approve on 2014-06-23
Review via email: mp+224013@code.launchpad.net

Commit message

Ensure the window is made fullscreen before it is shown, to work around an issue with calculating window coordinates.

Also ensure that the window has been shown before querying for MAX_GL_TEXTURE_SIZE.

Description of the change

Do not set fullscreen by default if we are not on Desktop.
This seems to get the wrong window height on devices and makes toolbar failing to work.
Also it seems unnecessary to set fullscreen on devices.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-06-21:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/gallery-app-ci/877/rebuild

review: Approve (continuous-integration)

Revision history for this message

Arthur Mello (artmello) wrote on 2014-06-21:

Are there any related MPs required for this MP to build/function as expected? Please list: No
Is your branch in sync with latest trunk (e.g. bzr pull lp:trunk -> no changes): Yes
Did you perform an exploratory manual test run of your code change and any related functionality on device or emulator?: Yes
Did you successfully run all tests found in your component's Test Plan (https://wiki.ubuntu.com/Process/Merges/TestPlan/gallery-app) on device or emulator? Yes
If you changed the UI, was the change specified/approved by design? No UI changes.
If you changed the packaging (debian), did you subscribe a core-dev to this MP? No packaging changes.

Revision history for this message

Oliver Grawert (ogra) wrote on 2014-06-23:

looks good

review: Approve

Revision history for this message

Olivier Tilloy (osomon) wrote on 2014-06-23:

This reverts a change that was introduced at revision 919 (3 months ago) and that was part of a bigger set of changes entitled "click support". There might have been a good reason for the gallery app to be run fullscreen on devices, so before reverting this, I would like to understand why it was like this in the first place.

Also, if we go ahead with this change, the comment a couple of lines above needs to be updated, as it says "run fullscreen if […] on a device".

Finally, a suggestion that you might want to try out: could it be that the problem happens because the view is first shown and then made fullscreen (thus messing the coordinates)? If that was the case, then I believe calling m_view->showFullScreen() might work around the issue.

review: Needs Fixing

Revision history for this message

Arthur Mello (artmello) wrote on 2014-06-23:

Yes, there is an issue removing fullscreen mode on devices. The photo viewer does not work properly (header is not removed, for example). I am checking what Olivier Tilloy mentioned about calling fullscreen after the first show.

Revision history for this message

Arthur Mello (artmello) wrote on 2014-06-24:

We have two problems that I tried to fix on this last commit. Both issues results on a window with problematic height values.

The first issue is related to set window state to fullscreen after calling view show, as Olivier mentioned. I changed the sequence of the calls.

The other issue is when calculating the maxTextureSize. To do this we call view->winID() to make sure the QPA is running. Calling this before showing the view results in wrong height values. I changed that sequence too.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-06-24:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/gallery-app-ci/878/rebuild

review: Approve (continuous-integration)

Revision history for this message

Gustavo Pichorim Boiko (boiko) wrote on 2014-06-24:

25 + // To define maxTextureSize we need to make sure QPA is running and that need to be called after show

Just a small typo: "that need to be" should read "that needs to be"

review: Needs Fixing

Revision history for this message

Gustavo Pichorim Boiko (boiko) wrote on 2014-06-24:

Did you perform an exploratory manual test run of the code change and any related functionality on device or emulator?
Yes

Did CI run pass? If not, please explain why.
Yes

Have you checked that submitter has accurately filled out the submitter checklist and has taken no shortcut?
Yes

Code looks good and works as expected!

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-06-25:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/gallery-app-ci/880/rebuild

review: Approve (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2014-06-25:

That seems to work, and fixes bug #1332632.

review: Approve

Revision history for this message

Olivier Tilloy (osomon) wrote on 2014-06-25:

I did some smoke testing on device, and fullscreen mode regressed there (the app is not actually launched fullscreen any longer). You probably need to do something like that:

    if (m_cmdLineParser->isFullscreen() || !isDesktopMode()) {
        setFullScreen(true);
        m_view->showFullScreen();
    } else {
        m_view->show();
    }

review: Needs Fixing

lp:~artmello/gallery-app/gallery-app-fix_toolbar_device updated on 2014-06-25

1005. By Arthur Mello on 2014-06-25: Show view fullscreen on devices

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-06-25:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/gallery-app-ci/881/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2014-06-25:

This works better now.

review: Approve

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:

Arthur Mello

Ubuntu Phablet Team

1	=== modified file 'src/gallery-application.cpp'
2	--- src/gallery-application.cpp 2014-06-10 20:00:44 +0000
3	+++ src/gallery-application.cpp 2014-06-25 17:20:34 +0000
4	@@ -203,8 +203,6 @@
5	rootContext->setContextProperty("DEVICE_WIDTH", QVariant(size.width()));
6	rootContext->setContextProperty("DEVICE_HEIGHT", QVariant(size.height()));
7	rootContext->setContextProperty("FORM_FACTOR", QVariant(m_cmdLineParser->formFactor()));
8	- rootContext->setContextProperty("MAX_GL_TEXTURE_SIZE",
9	- QVariant(m_galleryManager->resource()->maxTextureSize()));
10
11	// Set ourselves up to expose functionality to run external commands from QML...
12	m_view->engine()->rootContext()->setContextProperty("APP", this);
13	@@ -217,12 +215,17 @@
14	QObject::connect(this, SIGNAL(mediaLoaded()), rootObject, SLOT(onLoaded()));
15
16	//run fullscreen if specified at command line or not in DESKTOP_MODE (i.e. on a device)
17	- m_view->show();
18	-
19	if (m_cmdLineParser->isFullscreen() \|\| !isDesktopMode()) {
20	setFullScreen(true);
21	+ m_view->showFullScreen();
22	+ } else {
23	+ m_view->show();
24	}
25
26	+ // To define maxTextureSize we need to make sure QPA is running and that needs to be called after show
27	+ rootContext->setContextProperty("MAX_GL_TEXTURE_SIZE",
28	+ QVariant(m_galleryManager->resource()->maxTextureSize()));
29	+
30	if (m_cmdLineParser->startupTimer())
31	qDebug() << "GalleryApplication view created" << m_timer->elapsed() << "ms";
32	}
33
34	=== modified file 'src/gallery-manager.cpp'
35	--- src/gallery-manager.cpp 2014-05-28 13:42:14 +0000
36	+++ src/gallery-manager.cpp 2014-06-25 17:20:34 +0000
37	@@ -71,8 +71,6 @@
38	m_desktopMode(desktopMode),
39	m_mediaLibrary(0)
40	{
41	- const int maxTextureSize = m_resource->maxTextureSize();
42	- m_standardImageProvider->setMaxLoadResolution(maxTextureSize);
43	m_mediaFactory = new MediaObjectFactory(m_desktopMode, m_resource);
44
45	m_galleryManager = this;
46	@@ -283,6 +281,9 @@
47	*/
48	GalleryStandardImageProvider* GalleryManager::takeGalleryStandardImageProvider()
49	{
50	+ const int maxTextureSize = m_resource->maxTextureSize();
51	+ m_standardImageProvider->setMaxLoadResolution(maxTextureSize);
52	+
53	GalleryStandardImageProvider *provider = m_standardImageProvider;
54	m_standardImageProvider = 0;
55	return provider;