Code review comment for lp://staging/~abreu-alexandre/unity-webapps-qml/application-api

Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote :

> I tested it and it's mostly working. A few comments:
>
> getPlatformInfos: I think it's better to call it "getPlatformInfo" without the
> final "s". Anyway, this just returns the name of the QPA plugin, so I'd rather
> rename the method to "getPlatformName" or keep the current name and make it
> return a dictionary, currently having only one field "name" which contains the
> platform name. But I'd take the second approach only if you really plan to add
> more info there in the future.

done,

> The example expects the onActivated() and onDeactivated() signal to carry a
> parameter, but they don't.

done,

> All the methods are asynchronous and take a callback; even the
> applicationName() function is asynchronous, even if the result is immediately
> available. I'm not familiar with HTML5 development so this might be perfectly
> fine, I just point this out because coming from the C++ world this sounds
> rather strange (and a bit inconvenient).

right, agree, took another approach,

> Naming: it's better to use the same convention for all the getters: either
> start their name always with a "get", or never. Most of the methods you added
> start with "get", but "applicationName" doesn't.

done,

> getInputMethod(): it's not very clear what this function does; maybe rename it
> to "getInputMethodName"?

done,

> nameFromScreenOrientation(): you are only use two possible values, "Landscape"
> and "Portrait", while Qt supports 4. I guess that most apps don't care about
> distinguishing between the extra modes, but maybe some do (to be sure that the
> mic is on a specific side, for instance)?

right, agree, done

« Back to merge proposal