Oxide

Merge lp://qastaging/~osomon/oxide/touch-selection-api into lp://qastaging/~oxide-developers/oxide/oxide.trunk

  1. touch-selection-api
  2. Merge into oxide.trunk
Proposed by Olivier Tilloy
Status: Merged
Merged at revision: 1309
Proposed branch: lp://qastaging/~osomon/oxide/touch-selection-api
Merge into: lp://qastaging/~oxide-developers/oxide/oxide.trunk
Diff against target: 2496 lines (+1435/-54)
37 files modified
qt/core/browser/oxide_qt_browser_platform_integration.cc (+8/-3)
qt/core/browser/oxide_qt_browser_platform_integration.h (+1/-0)
qt/core/browser/oxide_qt_touch_handle_drawable.cc (+105/-0)
qt/core/browser/oxide_qt_touch_handle_drawable.h (+56/-0)
qt/core/browser/oxide_qt_web_context_menu.cc (+1/-0)
qt/core/browser/oxide_qt_web_view.cc (+58/-4)
qt/core/browser/oxide_qt_web_view.h (+13/-7)
qt/core/core.gyp (+4/-1)
qt/core/glue/oxide_qt_touch_handle_drawable_proxy.h (+56/-0)
qt/core/glue/oxide_qt_web_context_menu_proxy_client.h (+0/-11)
qt/core/glue/oxide_qt_web_view_proxy.h (+18/-4)
qt/core/glue/oxide_qt_web_view_proxy_client.h (+8/-1)
qt/qmlplugin/oxide.qmltypes (+28/-0)
qt/qmlplugin/oxide_qml_plugin.cc (+3/-0)
qt/quick/CMakeLists.txt (+3/-1)
qt/quick/api/oxideqquicktouchselectioncontroller.cc (+92/-0)
qt/quick/api/oxideqquicktouchselectioncontroller_p.h (+74/-0)
qt/quick/api/oxideqquickwebview.cc (+45/-1)
qt/quick/api/oxideqquickwebview_p.h (+10/-0)
qt/quick/api/oxideqquickwebview_p_p.h (+7/-1)
qt/quick/oxide_qquick_touch_handle_drawable.cc (+286/-0)
qt/quick/oxide_qquick_touch_handle_drawable.h (+70/-0)
qt/quick/oxide_qquick_web_context_menu.cc (+1/-0)
qt/tests/qmltests/api/tst_WebView_editingCapabilities.html (+6/-0)
qt/tests/qmltests/api/tst_WebView_editingCapabilities.qml (+132/-0)
shared/browser/oxide_browser_platform_integration.cc (+6/-0)
shared/browser/oxide_browser_platform_integration.h (+2/-0)
shared/browser/oxide_browser_platform_integration_observer.h (+3/-1)
shared/browser/oxide_render_widget_host_view.cc (+153/-2)
shared/browser/oxide_render_widget_host_view.h (+27/-1)
shared/browser/oxide_render_widget_host_view_container.h (+12/-1)
shared/browser/oxide_web_view.cc (+100/-4)
shared/browser/oxide_web_view.h (+18/-5)
shared/browser/oxide_web_view_client.cc (+10/-1)
shared/browser/oxide_web_view_client.h (+16/-1)
shared/renderer/oxide_content_renderer_client.cc (+1/-3)
shared/shared.gyp (+2/-1)
To merge this branch: bzr merge lp://qastaging/~osomon/oxide/touch-selection-api
Reviewer Review Type Date Requested Status
Chris Coulson Approve
Olivier Tilloy (community) Abstain
Review via email: mp+277193@code.qastaging.launchpad.net

Commit message

Add a touch selection API, to allow embedders to display handles for resizing the current selection, and contextual actions for it.

Description of the change

Initial proposal for a touch selection API.
Feedback on the proposed API and architecture welcome.

I shared a small example here for tests, along with screenshots of how this looks like on an MX4: http://people.canonical.com/~osomon/oxide-touch-selection/.

To post a comment you must log in.
lp://qastaging/~osomon/oxide/touch-selection-api updated
1274. By Olivier Tilloy

Report the actual selected text.

1275. By Olivier Tilloy

Rename selectionText to selectedText for consistency with chromium’s API.

1276. By Olivier Tilloy

Re-create the touch selection handles if the embedder modifies the component on the fly.

1277. By Olivier Tilloy

Add a 'bounds' property to the touch selection controller.

Revision history for this message
Olivier Tilloy (osomon) wrote :

This should now be ready for review.

Regarding this comment:

    // XXX: if editable, can we determine whether undo/redo is available?

It doesn’t look like the content API exposes a way to get at the currently focused editor, nor a canUndo() method of some sort.

Revision history for this message
Olivier Tilloy (osomon) wrote :

I tested further and I’m seeing one issue with the positioning of the handles and of the bounds when the locationBarController is used: the positions are offset by the height of the location bar.

review: Needs Fixing
lp://qastaging/~osomon/oxide/touch-selection-api updated
1278. By Olivier Tilloy

Take into account the location bar offset.

1279. By Olivier Tilloy

Update the bounding rect and positions of the handles if the location bar offset changes while a touch selection is active.

Revision history for this message
Olivier Tilloy (osomon) wrote :

The issue I was pointing out above should be fixed now.

review: Abstain
lp://qastaging/~osomon/oxide/touch-selection-api updated
1280. By Olivier Tilloy

Enable adaptive handle orientation, and expose the mirrorVertical and mirrorHorizontal properties.

1281. By Olivier Tilloy

Disable adaptive handle orientation for now, to match what chrome on android does.

1282. By Olivier Tilloy

Version 1.12 of the QML plugin corresponds to revision 7 of the WebView’s API.

1283. By Olivier Tilloy

Expose the horizontalPaddingRatio property on the handle component.

1284. By Olivier Tilloy

Merge the latest changes from trunk, and resolve a few conflicts.

1285. By Olivier Tilloy

Copy ConvertSelectionBound()’s implementation from content/browser/renderer_host/input/ui_touch_selection_helper.cc because that helper is not part of content’s public API.

Revision history for this message
Chris Coulson (chrisccoulson) wrote :

Sorry for taking ages on this. Whilst I continue to review the implementation, I do have some high-level comments on the API:

- OxideQQuickTouchSelectionController::editFlags would be better off as a property on WebView (as editingCapabilities or something), given that it has executeEditingCommand() there. This isn't unique to touch selection, and it's something that WebView needs anyway.
- The read only properties on OxideQQuickTouchSelection have setters in what will eventually be a public header. There shouldn't be public setters for these if they're only updated from Oxide.
- It's not clear what OxideQQuickTouchSelectionController::selectedText will be used for.
- OxideQQuickTouchSelectionController::Orientation should probably be OxideQQuickTouchSelectionController::HandleOrientation.
- Will horizontalPaddingRatio be used for our touch handles? It's always zero in Chrome/Aura and 0.25f in Chrome/Android. If we don't have a use for this in the design of our touch handles, I'd prefer this not to be exposed.

Revision history for this message
Olivier Tilloy (osomon) wrote :

> - Will horizontalPaddingRatio be used for our touch handles? It's
> always zero in Chrome/Aura and 0.25f in Chrome/Android. If we don't
> have a use for this in the design of our touch handles, I'd prefer
> this not to be exposed.

Yes. This is necessary if the arrow/pointer of the assets is not aligned to the right (for the left handle) / to the left (for the right handle). It seems the selection handles in the Ubuntu UITK are being redesigned at the moment, so there are no definite visuals yet, but I’ve seen some mockups where the arrow is horizontally centered, for which horizontalPaddingRatio needs to be 0.5f.

lp://qastaging/~osomon/oxide/touch-selection-api updated
1286. By Olivier Tilloy

Remove the selectedText read-only property from OxideQQuickTouchSelectionController,
it’s not really useful since the webview has cut/copy/paste commands which interact directly with the clipboard.

1287. By Olivier Tilloy

Rename OxideQQuickTouchSelectionController::Orientation to OxideQQuickTouchSelectionController::HandleOrientation.

1288. By Olivier Tilloy

Move OxideQQuickTouchSelectionController::editFlags to OxideQQuickWebView::editingCapabilities.

1289. By Olivier Tilloy

Remove setters for the 'active' and 'bounds' properties on OxideQQuickTouchSelectionController.

Revision history for this message
Olivier Tilloy (osomon) wrote :

All your high-level comments on the API should now be addressed/answered.

Revision history for this message
Olivier Tilloy (osomon) wrote :

I’m currently adding unit tests for WebView.editingCapabilities, and facing an issue: the current implementation emits editingCapabilitiesChanged whenever the selection bounds change, which seems to cover most use cases, but one: when the user issues the Copy command, the contents of the clipboard change, and the editing caps potentially change with it (if the clipboard was empty, the PasteCapability becomes true). Emitting editingCapabilitiesChanged in WebView::executeEditingCommand() just after calling contents::WebContents::Copy() doesn’t do the trick, as copying content to the clipboard is async (and thus by the time the signal is emitted the content might not have been copied to the clipboard yet).

In fact the problem is more general than that: if content is copied to the clipboard outside of the application, there’s no way to get notified of it and update the editing caps. There’s a comment in chrome/browser/ui/views/frame/browser_view.cc (above BrowserView::CutCopyPaste()) that suggests that this is currently not possible anyway.

Is WebView.editingCapabilities a good idea after all?

Revision history for this message
Chris Coulson (chrisccoulson) wrote :

> I’m currently adding unit tests for WebView.editingCapabilities, and facing an
> issue: the current implementation emits editingCapabilitiesChanged whenever
> the selection bounds change, which seems to cover most use cases, but one:
> when the user issues the Copy command, the contents of the clipboard change,
> and the editing caps potentially change with it (if the clipboard was empty,
> the PasteCapability becomes true). Emitting editingCapabilitiesChanged in
> WebView::executeEditingCommand() just after calling
> contents::WebContents::Copy() doesn’t do the trick, as copying content to the
> clipboard is async (and thus by the time the signal is emitted the content
> might not have been copied to the clipboard yet).
>
> In fact the problem is more general than that: if content is copied to the
> clipboard outside of the application, there’s no way to get notified of it and
> update the editing caps. There’s a comment in
> chrome/browser/ui/views/frame/browser_view.cc (above
> BrowserView::CutCopyPaste()) that suggests that this is currently not possible
> anyway.
>
> Is WebView.editingCapabilities a good idea after all?

We need some way of exposing this from WebView in order to support editing commands in an application menu. If possible, I'd like to not have this information duplicated in 3 different places (it's already exposed specifically for context menus).

Is the lack of notifications Chromium specific, or is that something we can get from Qt? (I'm not sure what QClipboard::dataChanged does)

Revision history for this message
Olivier Tilloy (osomon) wrote :

> Is the lack of notifications Chromium specific, or is that something
> we can get from Qt? (I'm not sure what QClipboard::dataChanged does)

It seems we can get the information from Qt. I just did a quick test with QClipboard (which is conveniently exposed in Qml by Ubuntu.Components, see https://developer.ubuntu.com/api/apps/qml/sdk-15.04/Ubuntu.Components.Clipboard/), and indeed the dataChanged signal is emitted whenever new data is being copied to the clipboard. Presumably, we can listen to this signal to emit editingCapabilitiesChanged. This also works when the application doesn’t have active focus.

lp://qastaging/~osomon/oxide/touch-selection-api updated
1290. By Olivier Tilloy

Emit editingCapabilitiesChanged() when the contents of the clipboard change.

1291. By Olivier Tilloy

Merge the latest changes from trunk and resolve a couple of minor conflicts.

1292. By Olivier Tilloy

Cache the edit flags to avoid emitting a signal when they haven’t actually changed.

1293. By Olivier Tilloy

Add unit tests for the WebView.editingCapabilities API.

Revision history for this message
Olivier Tilloy (osomon) wrote :

I added unit tests for the WebView.editingCapabilities API. Some of them are partially commented out as they would fail because of bug #1524288.

Revision history for this message
Chris Coulson (chrisccoulson) wrote :

Sorry for taking ages on this - I've added some comments inline. I'm still going through it though, so there might be a few more later.

review: Needs Fixing
Revision history for this message
Chris Coulson (chrisccoulson) wrote :

I've left another couple of comments

Revision history for this message
Chris Coulson (chrisccoulson) wrote :

Ok, I think I'm done for now (there's another couple of comments)

lp://qastaging/~osomon/oxide/touch-selection-api updated
1294. By Olivier Tilloy

Merge the latest changes from trunk.

1295. By Olivier Tilloy

Disconnect all signal handlers in oxide::qt::BrowserPlatformIntegration’s destructor.

1296. By Olivier Tilloy

Use RenderWidgetHostViewContainer instead of having RWHV call into WebView directly.

1297. By Olivier Tilloy

Do not explicitly call reset() on scoped pointers in a destructor, this is happening at destruction anyway.

1298. By Olivier Tilloy

Ensure the edit flags are updated when the RWHV changes.

1299. By Olivier Tilloy

Pass the bounds by const reference instead of by value.

1300. By Olivier Tilloy

Clear the edit flags if there’s no RWHV.

1301. By Olivier Tilloy

Factor out code in a helper function.

1302. By Olivier Tilloy

Remove the base oxide::TouchHandleDrawable implementation, and oxide::TouchHandleDrawableDelegate.
This was one layer too many for no good reason.

1303. By Olivier Tilloy

Merge the latest changes from trunk.

1304. By Olivier Tilloy

Fix a regression with vertical positioning of handles that crept in a previous commit.

1305. By Olivier Tilloy

Update copyright years in headers of changed files.

1306. By Olivier Tilloy

Remove explicit disconnections in destructor, which are handled automatically by QObject.

1307. By Olivier Tilloy

onTouchSelectionChanged doesn’t need to be a slot.

1308. By Olivier Tilloy

Move the pointer to the touch selection handle to OxideQQuickTouchSelectionControllerPrivate.

1309. By Olivier Tilloy

Make RWHV an implementation of ui::TouchSelectionControllreClient, instead of proxying calls to a separate object.

Revision history for this message
Olivier Tilloy (osomon) wrote :

Thanks for the thorough review. I think I addressed all your comments.

Revision history for this message
Chris Coulson (chrisccoulson) wrote :

This looks fine to merge now - I'll probably report a few follow up bugs with other cleanups / suggestions, but those don't need to block the initial landing

review: Approve
lp://qastaging/~osomon/oxide/touch-selection-api updated
1310. By Olivier Tilloy

Merge the latest changes from trunk and resolve a minor conflict.

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