Ubuntu Document Viewer App

Merge lp://staging/~verzegnassi-stefano/ubuntu-docviewer-app/fix-documents-page-keyboard into lp://staging/ubuntu-docviewer-app

  1. fix-documents-page-keyboard
  2. Merge into lo-viewer
Proposed by Stefano Verzegnassi
Status: Merged
Approved by: Stefano Verzegnassi
Approved revision: 332
Merged at revision: 336
Proposed branch: lp://staging/~verzegnassi-stefano/ubuntu-docviewer-app/fix-documents-page-keyboard
Merge into: lp://staging/ubuntu-docviewer-app
Diff against target: 29 lines (+4/-0)
1 file modified
src/app/qml/documentPage/DocumentPage.qml (+4/-0)
To merge this branch: bzr merge lp://staging/~verzegnassi-stefano/ubuntu-docviewer-app/fix-documents-page-keyboard
Reviewer Review Type Date Requested Status
Nekhelesh Ramananthan (community) Approve
Jenkins Bot continuous-integration Approve
Ubuntu Document Viewer Developers Pending
Review via email: mp+291139@code.staging.launchpad.net

Commit message

Fixed the keyboard input issues in DocumentsPage, caused by the migration to the new PageHeader component. In particular:
* OSK visible on app start-up
* It was possible to type chars into the searchField (and then filter the document entries out) even if the searchFiled was not visible
* OSK still visible after the search header was closed (through the 'Cancel' button)

Description of the change

Fixed the keyboard input issues in DocumentsPage, caused by the migration to the new PageHeader component. In particular:
* OSK visible on app start-up
* It was possible to type chars into the searchField (and then filter the document entries out) even if the searchFiled was not visible
* OSK still visible after the search header was closed (through the 'Cancel' button)

To post a comment you must log in.
Revision history for this message
Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :

PASSED: Continuous integration, rev:331
https://core-apps-jenkins.ubuntu.com/job/docviewer-app-ci/271/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/853/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/docviewer-app-ci/271/rebuild

review: Approve (continuous-integration)
Revision history for this message
Nekhelesh Ramananthan (nik90) wrote :

Why not load the textfield dynamically. This way when it is loaded, it automatically gets focus and shows OSK, and when it is unloaded, the OSK also hides. And you save a tiny memory footprint :P. Also this way you don't need to force focus on the scrollView on component.onCompleted, thereby removing the workarounds.

review: Needs Information
Revision history for this message
Stefano Verzegnassi (verzegnassi-stefano) wrote :

UITK docs are not very clear on what to do when a Page have multiple PageHeaders.
I used the clock-app code (more specifically, the alarm page) as reference, since you already completed the migration to UC 1.3 at the time.

I'm not sure about the use of QML Loaders, since it would look a bit "dirty" to me.
Well, it's not required to have the other headers loaded by default - that's for sure - but at the moment we still have some logic inside the headers and I'd like to avoid to add further bindings here and there.

However, I just gave a second check to the QtQuick docs, and my fix is actually wrong.

A not-visible Item does not receive mouse inputs, but it's still able to receive keyboard events and grab the focus.[1]
We rather need to set 'enabled: visible' in order to prevent that.

A small question:

With the older PageHeadState, the content of an header was unloaded as long as the Page didn't change to that specific state. With the newer PageHeader, a different approach has been used, and anything can be customized as the single developer needs.

Anyway, there are a few behaviours that should be forced, and an inactive (i.e. not visible) header should not receive any event by default.
Do you think it's worth to report a bug to the UITK project?

[1] http://doc.qt.io/qt-5/qquickitem.html#visible-prop

lp://staging/~verzegnassi-stefano/ubuntu-docviewer-app/fix-documents-page-keyboard updated
332. By Stefano Verzegnassi

Properly fix keyboard focus of PageHeader(s)

Revision history for this message
Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :

PASSED: Continuous integration, rev:332
https://core-apps-jenkins.ubuntu.com/job/docviewer-app-ci/274/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/862/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/docviewer-app-ci/274/rebuild

review: Approve (continuous-integration)
Revision history for this message
Nekhelesh Ramananthan (nik90) wrote :

I suppose disabling them when they are not visible seems appropriate and works nicely. Frankly I would like to see the SDK provide a way to unload unused headers either by supporting QML Loaders, since these kind of issues shouldn't be there in the first place.

review: Approve
Revision history for this message
Stefano Verzegnassi (verzegnassi-stefano) wrote :

I will mind to open a bug at ubuntu-ui-toolkit. I'm really missing the old PageHeadState behaviour too... :/
Thanks again for the review!

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