New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use QLabels to draw hints #1868

Merged
merged 17 commits into from Aug 18, 2016

Conversation

Projects
None yet
4 participants
@The-Compiler
Copy link
Collaborator

The-Compiler commented Aug 17, 2016

This is a complete rewrite of how hints are drawn, which now use native Qt drawing and QLabels instead of injecting elements into the QWebView. This has various benefits:

  • Fixes issues like #925 and #1126 where the hint drawing is influenced by the page
  • Lets us use a single coordinate system for drawing and clicking hints, which simplifies webelem.rect_on_view.
  • Lets us get rid of various webelem methods which aren't needed anymore now, which in turn
  • Gets us a step closer to having hints with QtWebEngine.

As an effect of that, how e.g. gradients and the label opacity is configured changes. I've tried to migrate the most common cases, but this will probably require some manual intervention for some users.

Since this is quite a big change with some potential of breakage, I'd appreciate some review. @Kingdread @rcorre @lahwaacz @haasn want to help or try the branch out locally? FWIW, it might be easiest to review this commit by commit.


This change is Reviewable

The-Compiler added some commits Aug 16, 2016

Use native QLabels for hints
This will make labels work easily with QtWebEngine, and make sure
they're not affected by the page's contents.

Fixes #925.
Fixes #1126.
Remove no longer needed webelem methods
The following methods were only used for hint labels and thus removed
now:

- document_element
- create_inside
- find_first
- set_inner_xml
- remove_from_document
- set_style_property
Add styling of new hints
This also removes the hints -> opacity setting, as this is now set by
using an rgba(...) color.
Remove adjust_zoom for webelem.rect_on_view
Previously, the drawn hint labels were affected by the zoom, i.e., they
were stretched out by QtWebKit and actually had to be drawn at the
unzoomed position.

The Python/C++ API gives us coordinated adjusted for zoom, so
we always *negatively* adjusted them to get the unzoomed coordinates.

JS gave us the original coordinates, so we stretched them out according
to the zoom if adjust_zoom was given (which means only when clicking a
link).

Now we always operate in term of display coordinates: The point where we
draw the hint label is equal to the point we're clicking.

Thus, the zoom level for javascript is always adjusted, and the Python
zoom level is never (negatively) adjusted.
Don't fully clean up labels if their frame is gone
Otherwise they're invalid but still in the HintContext, so calling
.hide() on it later (e.g. because the user pressed another key) would
give us a RuntimeError from PyQt.
Remove logging in _on_contents_size_changed
Otherwise this will be logged once for every element.
qtutils.ensure_valid(start_color)
qtutils.ensure_valid(stop_color)

return ('qlineargradient(x1:0, y1:0, x2:1, y2:1, stop:0 {}, '

This comment has been minimized.

@Kingdread

Kingdread Aug 17, 2016

Collaborator

Not that it matters much, but this conversation is not 100% correct. It changes from a gradient top-bottom to a gradient topleft-bottomright, which means it now as a 45° angle compared to before. The correct parameter would be x2:0, so that the interpolation line goes from (0, 0) to (0, 1), which is top-bottom without angle.

You can barely see it with standard settings, but changing the gradient to black-white and the size to 50px should show you the difference.

This comment has been minimized.

@The-Compiler

The-Compiler Aug 17, 2016

Collaborator

No idea how you managed to catch that, but fixed 😆

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 17, 2016

Current coverage is 81.73% (diff: 86.86%)

Merging #1868 into master will increase coverage by 0.06%

@@             master      #1868   diff @@
==========================================
  Files           120        120          
  Lines         16789      16738    -51   
  Methods           0          0          
  Messages          0          0          
  Branches       2562       2559     -3   
==========================================
- Hits          13711      13680    -31   
+ Misses         2605       2581    -24   
- Partials        473        477     +4   

Powered by Codecov. Last update ea14b5b...8eaa387

@The-Compiler The-Compiler force-pushed the new-hints branch from a169a11 to 8eaa387 Aug 17, 2016

@The-Compiler The-Compiler merged commit 8eaa387 into master Aug 18, 2016

6 checks passed

codecov/patch 86.86% of diff hit (target 81.66%)
Details
codecov/project 81.73% (+0.06%) compared to ea14b5b
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@The-Compiler

This comment has been minimized.

Copy link
Collaborator

The-Compiler commented Aug 18, 2016

Merged - let's see what will break 😉

@The-Compiler The-Compiler deleted the new-hints branch Jan 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment