[FIX]Silhouette Plot: elide hover labels if labels are long #2278
Conversation
jerneju
changed the title
jerneju
changed the title
Codecov Report
@@ Coverage Diff @@
## master #2278 +/- ##
==========================================
+ Coverage 73.27% 73.32% +0.05%
==========================================
Files 317 317
Lines 55384 55382 -2
==========================================
+ Hits 40581 40611 +30
+ Misses 14803 14771 -32 |
| @@ -507,7 +504,10 @@ def setRowNames(self, names): | |||
| item = layout.itemAt(i + 1, 3) | |||
|
|
|||
| if grp.rownames is not None: | |||
| item.setItems(grp.rownames) | |||
| rownames = [rowname[0:12] + "..." if | |||
There was a problem hiding this comment.
Use ellipsis symbol instead: …
|
Is this still WIP? |
jerneju
changed the title
| @@ -907,7 +907,7 @@ def event(self, event): | |||
| return super().event(event) | |||
|
|
|||
| def sizeHint(self, which, constraint=QSizeF()): | |||
| spacing = max(self.__spacing * (self.count() - 1), 0) | |||
| spacing = max(self.__spacing * (self.count()), 0) | |||
There was a problem hiding this comment.
Why extra parentheses?
Can self.__spacing * self.count() (legally) be negative? Why max then?
| @@ -907,7 +907,7 @@ def event(self, event): | |||
| return super().event(event) | |||
|
|
|||
| def sizeHint(self, which, constraint=QSizeF()): | |||
| spacing = max(self.__spacing * (self.count() - 1), 0) | |||
| spacing = max(self.__spacing * (self.count()), 0) | |||
| return QSizeF(300, self.__barsize * self.count() + spacing) | |||
There was a problem hiding this comment.
Isn't this (self.__barsize + self.__spacing) * self.count()?
| @@ -507,7 +504,10 @@ def setRowNames(self, names): | |||
| item = layout.itemAt(i + 1, 3) | |||
|
|
|||
| if grp.rownames is not None: | |||
| item.setItems(grp.rownames) | |||
| rownames = [rowname[0:12] + "…" if | |||
There was a problem hiding this comment.
rowname[:12] instead of rownames[0:12]
There was a problem hiding this comment.
However, this is not the proper way to do it since it disregards the actual widths of characters. See if you can truncate the text with http://doc.qt.io/qt-4.8/qfontmetrics.html#elidedText instead. For this, you will have to decide for (instead of determining) the sensible maximal label width in advance.
| rownames = [rowname[0:12] + "…" if | ||
| len(rowname) > 15 else rowname | ||
| for rowname in grp.rownames] | ||
| item.setItems(rownames) |
There was a problem hiding this comment.
Can you put the untruncated (or less truncated) label in the tooltip?
| @@ -1047,8 +1047,8 @@ def __naturalsh(self): | |||
| fm = QFontMetrics(self.font()) | |||
| spacing = self.__spacing | |||
| N = len(self.__items) | |||
| width = max((fm.width(text) for text in self.__items), | |||
| default=0) | |||
| width = 1.5 * max((fm.width(text) for text in self.__items), | |||
| @@ -1111,8 +1111,10 @@ def remove(items, scene): | |||
| self.__textitems = [] | |||
|
|
|||
|
|
|||
| def main(argv=sys.argv): | |||
| def main(argv=None): | |||
There was a problem hiding this comment.
I'd just skip the argument and always give an empty list to QApplication. Nobody calls this main except for this module itself.
| @@ -494,6 +494,7 @@ def setScores(self, scores, labels, values, colors, rownames=None): | |||
| self.__setup() | |||
|
|
|||
| def setRowNames(self, names): | |||
| ROW_NAMES_WIDTH = 200 | |||
There was a problem hiding this comment.
Introducing a constant is OK since you explain what 200 is. But having it here is almost as if it just above the line where it's used. What about moving this constant to module- or class-level (https://www.python.org/dev/peps/pep-0008/#constants)?
There was a problem hiding this comment.
So it's better to have code removed further away from where it's used, in this case in one single, particular place?
The ridicule is rhetorical.
There was a problem hiding this comment.
There are some widgets that have all their settings (like widths, various colors...) collected as constants at the top, which is convenient when you want to improve its design. Moving a single constant is the first step in this direction. :)
| @@ -907,8 +911,8 @@ def event(self, event): | |||
| return super().event(event) | |||
|
|
|||
| def sizeHint(self, which, constraint=QSizeF()): | |||
| spacing = max(self.__spacing * (self.count() - 1), 0) | |||
| return QSizeF(300, self.__barsize * self.count() + spacing) | |||
| spacing = max(self.__spacing, 0) | |||
There was a problem hiding this comment.
Why max(self.__spacing, 0)? Can self.__spacing be negative? I suspect this code was here because of (self.count() - 1).
|
@janezd |
Issue
Fixes #2162
Description of changes
Includes