Skip to content
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

[FIX] boxplot labels overlap #3011

Merged
merged 6 commits into from
May 11, 2018
Merged

Conversation

astaric
Copy link
Member

@astaric astaric commented Apr 19, 2018

Issue

When labels are wider than their respective bars in a boxplot, labels overlap:

screen shot 2018-04-19 at 15 39 12

Description of changes

Limit (all but the last) label width to the width of the bar they are labelling. Hide labels for very short bars (unless the whole label can be displayed in available space).

screen shot 2018-04-19 at 20 49 11

Includes
  • Code changes
  • Tests
  • Documentation

@codecov-io
Copy link

codecov-io commented Apr 20, 2018

Codecov Report

Merging #3011 into master will decrease coverage by 0.03%.
The diff coverage is 87.65%.

@@            Coverage Diff             @@
##           master    #3011      +/-   ##
==========================================
- Coverage   82.19%   82.16%   -0.04%     
==========================================
  Files         335      334       -1     
  Lines       57688    57713      +25     
==========================================
+ Hits        47417    47420       +3     
- Misses      10271    10293      +22


if self.__max_width is None or self.boundingRect().width() < self.__max_width:
return

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ales-erjavec Do you perhaps have any ideas how to limit label painting to some rect? So that we would not need to draw white rects over label parts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • set clip rect on the painter
  • actually implement bounded text painting (QPainter has quite a few drawText overloads for bounded text painting, also see QFontMetrics.elidedText)

@markotoplak
Copy link
Member

@astaric, I tried to clip instead of draw. Could you see if it still works as expected?

@astaric
Copy link
Member Author

astaric commented Apr 20, 2018

@markotoplak, while clip works, eliding works better

screen shot 2018-04-20 at 14 24 09

@lanzagar
Copy link
Contributor

Otherwise good, I just noticed that the last label can extend beyond the graph (to the right):
screenshot_18-05-11_11 46 41

Which even cuts off the text when the window size is smaller...
screenshot_18-05-11_11 46 08

Needed to ensure paint method gets executed
@@ -972,6 +1027,44 @@ def send_report(self):
if text:
self.report_caption(text)

class Label(QGraphicsSimpleTextItem):
"""Boxplot Label with settable maxWidth"""
Copy link
Contributor

@lanzagar lanzagar May 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move something like this to the gui module, so it can be reused elsewhere?

edit: not gui as that has elements for gui construction, but maybe somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When anyone else needs a label like this, it can be moved and imported from there. Until then, I see no point in further polluting the gui namespace.

@astaric
Copy link
Member Author

astaric commented May 11, 2018

The label extending to the right was a "feature", but since cropping of the visualization does not account for the label, I have removed it.

@astaric
Copy link
Member Author

astaric commented May 11, 2018

No idea why coverage does not get updated, but the results (https://codecov.io/gh/biolab/orange3/pull/3011/diff) indicate that diff coverage is at 96.39% :)

@ajdapretnar
Copy link
Contributor

Yolo.

@ajdapretnar ajdapretnar merged commit b62d04f into biolab:master May 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants