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

Write status message in single line in Qt toolbar. #4329

Merged
merged 1 commit into from Oct 20, 2015

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Apr 12, 2015

Currently, the status message is partially hidden when embedding a
Toolbar in a Qt4 app (see e.g. embedding_in_qt4_wtoolbar.py: activate
the pan tool and hover the mouse over the figure; the two lines of the
status messages don't fit in the toolbar).

The split-line message was introduced in 3c61684 to avoid automatic
horizontal expansion of the toolbar in presence of long messages (I
guess) but I cannot reproduce that issue right now.

Currently, the status message is partially hidden when embedding a
Toolbar in a Qt4 app (see e.g. embedding_in_qt4_wtoolbar.py: activate
the pan tool and hover the mouse over the figure; the two lines of the
status messages don't fit in the toolbar).

The split-line message was introduced in 3c61684 to avoid automatic
horizontal expansion of the toolbar in presence of long messages (I
guess) but I cannot reproduce that issue right now.
@mfitzp
Copy link
Member

mfitzp commented Apr 12, 2015

I have found a reference to the issue of QLabel resizing it's parent object here which sounds like the same problem the 3c61684 was trying to fix. If the error isn't seen any longer perhaps a Qt4 update has 'fixed' it, will have a check (+ with PyQt5, PySide, etc.).

If it's still reproducible it should be fixable by applying correct/additional size policies to the widgets.

@mfitzp
Copy link
Member

mfitzp commented Apr 12, 2015

I can't replicate the horizontal expansion bug from 3c61684 in latest PyQt5 (Qt 5.4.1) and PyQt4, PySide (Qt 4.8.6) on MacOS X. I'll check Windows/Linux tomorrow in case of platform weirdness.

An alternative to manual line-wrapping is .setWordWrap(True) on the label. In combination with setting a 12pt font this gives three lines of info in the toolbar (enough for the mode, x and y to sit in). I also tried using QFontMetrics to auto-ellipsis longer text, but that doesn't play nicely with wrapped text.

@tacaswell tacaswell added this to the proposed next point release milestone Apr 13, 2015
@mfitzp
Copy link
Member

mfitzp commented Apr 14, 2015

I can confirm the old expanding window bug no longer occurs on PyQt4 or PyQt5 (on Qt 4.8.6 and Qt 5.4.1) on Windows. So I think it is safe to revert this change. However, the result is still kind of ugly, so I think it might be worth trying to improve that while we're looking here. There are two options:

  1. Keep the wrapped text, but with additional removal of whitespace + shrink the font so 3 lines fit in the space.
  2. Set the text on a single line and elide so rather than disappearing the text is chopped(...)

The code for wrapping and stripping is as follows:

def set_message(self, s):
    self.message.emit(s)
    if self.coordinates:
        l = s.strip().split(',')
        l[-1:] = l[-1].split()
        self.locLabel.setText('\n'.join(l))

The use of .split() with no parameter allows us to drop multiple whitespace. The font setting (~line 618) is:

        f = self.locLabel.font()
        f.setPointSize(f.pointSize()-1)   # Or set to 12pt directly
        self.locLabel.setFont(f)

The result for this is:
screen shot 2015-04-14 at 10 19 59

The text-eliding option can be done with:

def set_message(self, s):
    self.message.emit(s)
    if self.coordinates:
        m = QtGui.QFontMetrics(self.locLabel.font())
        s = m.elidedText(s, QtCore.Qt.ElideRight, self.locLabel.width())
        self.locLabel.setText(s)

This looks like:
screen shot 2015-04-14 at 10 26 01

@anntzer what do you think to either of these?

@anntzer
Copy link
Contributor Author

anntzer commented Apr 14, 2015

I think shrinking is by far nicer than elision, but

  1. on KDE, this is seems to be not enough -- the text is still higher than the toolbar. Can someone confirm this? A Windows tester would be useful too.
  2. the "zoom rect" text (magnifying glass) gets incorrectly broken over two lines (most practical (short of passing lines of strings along) would probably be to use a tab internally as a separator).

@OceanWolf
Copy link
Contributor

@mfitzp Not sure how much work should go into this as MEP22 should completely revolutionise this, ensuring that the toolbar goes on a completely separate line so as to leave more room for tools. so I feel inclined to say that we should fix the example file itself rather than Qt.

@tacaswell can you clarify the timetable for MEP22? I.e. will the rcParam controlling its use change straight away, or make the default rcParam switch-over at a later day. If at a later date, will we actively promote that people switch to MEP22? I ask because I guess the answers will decide whether we update the embedding docs to use MEP22 now, or at that later date.

@tacaswell
Copy link
Member

@mdboom and @efiring need to weigh in on the time line. We definitely need
the new toolbar working with all backends before it becomes the default and
should probably go through a cycle of it being optional to get more through
real-life testing before it is made the default.

I know it is annoyingly conservative, but we can't break existing user code.

On Tue, Apr 14, 2015 at 11:35 AM OceanWolf notifications@github.com wrote:

@mfitzp https://github.com/mfitzp Not sure how much work should go into
this as MEP22 should completely revolutionise this, ensuring that the
toolbar goes on a completely separate line so as to leave more room for
tools. so I feel inclined to say that we should fix the example file itself
rather than Qt.

@tacaswell https://github.com/tacaswell can you clarify the timetable
for MEP22? I.e. will the rcParam controlling its use change straight away,
or make the default rcParam switch-over at a later day. If at a later date,
will we actively promote that people switch to MEP22? I ask because I guess
the answers will decide whether we update the embedding docs to use MEP22
now, or at that later date.


Reply to this email directly or view it on GitHub
#4329 (comment)
.

@OceanWolf
Copy link
Contributor

@tacaswell No problem, just wondered when should we change the embedding examples?

For example I now see we didn't convert https://github.com/matplotlib/matplotlib/blob/master/examples/user_interfaces/embedding_in_gtk3_panzoom.py during #3652.

Anyway, from your comment I see whatever happens it won't get made default straight away, so that just leaves the question do we want to encourage people to switch early, i.e. by changing the embedding examples as the backends?

Basically I see it as, without MEP22 the embed example for QT should get rewritten to follow the pattern FigureManagerQT which works and uses a pattern 99% similar to the one that MEP22 adopts. However if it looks like the example will get rewritten again before it makes it to a release, then I would suggest saving it and only doing it the once (only color release between MEP22 release).

(P.S. Definitely agree on getting all backends converted first, but for now I work on the assumption that we will get that done for the next point release)

@WeatherGod
Copy link
Member

is this PR finished? are there outstanding issues?

@WeatherGod WeatherGod added the status: needs clarification Issues that need more information to resolve. label Jul 12, 2015
@anntzer
Copy link
Contributor Author

anntzer commented Jul 12, 2015

I agree with @mfitzp that the result is a bit ugly but it is certainly more usable than the status quo, and less ugly than getting "zoom rect" broken into two lines.

@OceanWolf
Copy link
Contributor

@anntzer which option do you think we should use, do we need any changes to this PR? As I said above the whole toolbar will get a complete overhaul soon, so this will only act as a short-term fix for those using the current soon to become old toolbar...

@WeatherGod
Copy link
Member

Keep in mind "soon" will likely be the next point release after 2.0. Which
will likely be a year from now. Getting this in today if it doesn't break
anything and is an improvement is a good thing.

On Mon, Jul 13, 2015 at 4:16 PM, OceanWolf notifications@github.com wrote:

@anntzer https://github.com/anntzer which option do you think we should
use, do we need any changes to this PR? As I said above the whole toolbar
will get a complete overhaul soon, so this will only act as a short-term
fix for those using the current soon to become old toolbar...


Reply to this email directly or view it on GitHub
#4329 (comment)
.

@tacaswell
Copy link
Member

We should err on the side of short term fixes here. I suspect we are not
going to get mep 27 in for 1.5 (due to failures on the review side of
things), it will go out in 2.1 in April.

On Mon, Jul 13, 2015, 4:16 PM OceanWolf notifications@github.com wrote:

@anntzer https://github.com/anntzer which option do you think we should
use, do we need any changes to this PR? As I said above the whole toolbar
will get a complete overhaul soon, so this will only act as a short-term
fix for those using the current soon to become old toolbar...


Reply to this email directly or view it on GitHub
#4329 (comment)
.

@anntzer
Copy link
Contributor Author

anntzer commented Jul 14, 2015

I'll stick with the original PR, which is less ugly imho...

@WeatherGod
Copy link
Member

Just had a thought. How does this look now with the new feature showing the image value on mouseover of an image?

@anntzer
Copy link
Contributor Author

anntzer commented Oct 20, 2015

Kindly asking whether this should go to 1.5 as a short term fix (perhaps it's too late now?) or just be subsumed by MEP27 (feel free to close this and reopen a similar issue on MEP27's side if this is the case).

@mdboom
Copy link
Member

mdboom commented Oct 20, 2015

I don't think we need to wait on MEP27 for this. Have you had a chance to investigate @WeatherGod's question?

How does this look now with the new feature showing the image value on mouseover of an image?

@anntzer
Copy link
Contributor Author

anntzer commented Oct 20, 2015

With the old code, the x, y and image values are still cropped and half visible below the "pan/zoom" text. With the PR, the text can sometimes be too long to fit in a single line, depending on the width of the window (I guess it was already the issue before but it didn't appear with the default size used in the embedding_in_qt4_wtoolbar example), in which case no text appears, but this can easily be fixed by making the window wider (which is easier and probably more justifiable from an UI POV than making the toolbar thicker).

@mdboom
Copy link
Member

mdboom commented Oct 20, 2015

Thanks!

mdboom added a commit that referenced this pull request Oct 20, 2015
Write status message in single line in Qt toolbar.
@mdboom mdboom merged commit 50f373f into matplotlib:master Oct 20, 2015
mdboom added a commit that referenced this pull request Oct 20, 2015
Write status message in single line in Qt toolbar.
@mdboom
Copy link
Member

mdboom commented Oct 20, 2015

Backported to v1.5.x as 50f373f

@anntzer anntzer deleted the qt-toolbar-oneline-message branch October 20, 2015 20:16
@QuLogic QuLogic modified the milestones: next point release (1.5.0), proposed next point release (2.1) Oct 21, 2015
@QuLogic QuLogic removed the status: needs clarification Issues that need more information to resolve. label Nov 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants