Code review comment for lp:~gerboland/webbrowser-app/formFactor-support

Revision history for this message
Gerry Boland (gerboland) wrote :

> I’m not seeing any formFactor property defined on UserAgent02, so I doubt the
> onFormFactorChanged handler in UbuntuWebContext.qml would work.

Code untested. Was struggling to cross compile & deploy, sbuild unhappy with me today.

> Note that I’m currently working on a branch that removes the 'formFactor'
> property from the Ubuntu.Web plugin’s context object¹, so it’s kind of
> ironical that you’re adding it back in another branch. I really don’t think
> the default user agent (and corresponding overrides) should be tied to the
> "form factor" (whatever that means in a world of convergence). How does mir
> determine whether a device is a phone or a desktop, or a tablet? Is that a
> solid API that we are going to support and publicize?

I obviously have no idea of your plans, this was just an attempt at replacing your screen heuristic with something more concrete.

Form Factor will be a hint to the application to tell it what mode the shell is in, on each screen. Mir supports it, shell will do its best to set it correctly. I would hope the average app author has no need for such a property, any form factor specific behaviour would be encapsulated in the UITK.

> Even if that was a sensible thing to do, we most probably don’t want to get a
> mobile UX on a tablet. Well, at least not a 10" tablet. On a 7" tablet maybe.
> See where the screenDiagonal property comes from?

Is your call. The goal of this was to indicate that you can learn if the app is running on a tablet or a desktop UI at the time.

> As a side note, the dependency on Qt5Gui_PRIVATE is not very welcome, given
> that we’ve been steadily working towards entirely removing the dependencies on
> private Qt packages (ask Mirv about it).

Qt doesn't have a nice way to export it to apps yet, hence having to resort to private Qt headers. As I mentioned in a comment, a better solution is for the UITK to do that for you. This isn't the final solution.
Thanks for the comments!

« Back to merge proposal