[FIX]Silhouette Plot: elide hover labels if labels are long#2278
[FIX]Silhouette Plot: elide hover labels if labels are long#2278janezd merged 1 commit intobiolab:masterfrom jerneju:gh-2162-silhouette-labels
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 |
|
|
||
| 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
|
|
||
| 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?
| 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()?
|
|
||
| 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?
| 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), |
|
|
||
|
|
||
| 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.
janezd
left a comment
janezd
left a comment
There was a problem hiding this comment.
This is cool. Two really minor (potential) changes.
Plus, can you suggest a good data set to try this on?
| 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. :)
| 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 |
5 participants
Issue
Fixes #2162
Description of changes
Includes