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

[qt] Use monospace font #6864

Merged
merged 2 commits into from Nov 4, 2015
Merged

Conversation

@MarcoFalke
Copy link
Member

MarcoFalke commented Oct 21, 2015

This uses #4228 to find a monospace font for the qt rpcconsole.

(This replaces, thus closes #6781.)

@laanwj
laanwj reviewed Oct 21, 2015
View changes
src/qt/rpcconsole.cpp Outdated
@@ -463,13 +463,15 @@ void RPCConsole::clear()

// Set default style sheet
ui->messagesWidget->document()->setDefaultStyleSheet(
QString::fromStdString(strprintf(

This comment has been minimized.

Copy link
@laanwj

laanwj Oct 21, 2015

Member

Please use Qt's formatting, instead of strprintf. e.g.

QString("....\1...." ).arg(...)
@laanwj laanwj added the GUI label Oct 21, 2015
@MarcoFalke MarcoFalke force-pushed the MarcoFalke:MarcoFalke-2015-qtMonospace branch Oct 21, 2015
@jonasschnelli
Copy link
Member

jonasschnelli commented Oct 21, 2015

Binaries are here: https://bitcoin.jonasschnelli.ch/pulls/6864/

Would be good if we get some screenshot from HiDPI Ubuntu and retina OSX.

@Diapolo
Diapolo reviewed Oct 21, 2015
View changes
src/qt/rpcconsole.cpp Outdated
"td.cmd-request { color: #006060; } "
"td.cmd-error { color: red; } "
"b { color: #006060; } "
);

).arg(QFont("Monospace").pointSize() * 4 / 5) // 0.8 em

This comment has been minimized.

Copy link
@Diapolo

Diapolo Oct 21, 2015

QFont("Monospace").pointSize() * 4 / 5 is a float, so should this also include a call to QString & QString::setNum(float n, char format = 'g', int precision = 6)?

Also not sure if relevant, but as per the docs .pointSize():
Returns the point size of the font. Returns -1 if the font size was specified in pixels.

Perhaps you also could exchange Monospace with QFont::Monospace... I didn't try the code btw.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Oct 21, 2015

Author Member

Thanks, I have alrady replaced the code with a version that should work on all platforms (and maybe even HiDPI).

}

This comment has been minimized.

Copy link
@Diapolo

Diapolo Oct 21, 2015

Nit: Unwanted change

@Diapolo
Diapolo reviewed Oct 21, 2015
View changes
src/qt/rpcconsole.cpp Outdated
@@ -461,15 +461,30 @@ void RPCConsole::clear()
platformStyle->SingleColorImage(ICON_MAPPING[i].source).scaled(ICON_SIZE, Qt::IgnoreAspectRatio, Qt::SmoothTransformation));
}

QFont fontMatch("Monospace");

This comment has been minimized.

Copy link
@Diapolo

Diapolo Oct 21, 2015

What about the QFont::Monospace hint from a few minutes ago, seems clearer?

@dexX7
dexX7 reviewed Oct 22, 2015
View changes
src/qt/rpcconsole.cpp Outdated
"td.cmd-request { color: #006060; } "
"td.cmd-error { color: red; } "
"b { color: #006060; } "
);

).arg(fi.family(), "10pt")

This comment has been minimized.

Copy link
@dexX7

dexX7 Oct 22, 2015

Contributor

Doesn't a fixed font size bring back the issue addressed via #5898?

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Oct 22, 2015

Member

IIRC the issue from #5898 was mainly the px based font size.
Switching back to "Monospace" could be possible, but needs broad testing and also acceptance.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Oct 22, 2015

Author Member

@jonasschnelli Agree on your comment. Would you mind shooting up some new builds for this PR? =)

@dexX7
dexX7 reviewed Oct 22, 2015
View changes
src/qt/rpcconsole.cpp Outdated
QFont fontMatch("Monospace");
fontMatch.setStyleHint(QFont::TypeWriter);
QFontInfo fi(fontMatch);
assert(fi.fixedPitch());

This comment has been minimized.

Copy link
@dexX7

dexX7 Oct 22, 2015

Contributor

I personally think using an assertion in non-consensus code is too "strong".

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Oct 22, 2015

Author Member

Just ignore everything i put in the {//Debug} block for code review. ;)

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:MarcoFalke-2015-qtMonospace branch Oct 22, 2015
@laanwj
Copy link
Member

laanwj commented Oct 22, 2015

Looks good here. There used to be problems with (absurdly large) font size in Ubuntu with previous monospace patches, but I'm not seeing that now.
schermafdruk van 2015-10-22 17 45 27
Can't test hidpi though...

@jonasschnelli
Copy link
Member

jonasschnelli commented Oct 22, 2015

Kicked the build server,... new builds should be available within the next 30mins: https://bitcoin.jonasschnelli.ch/pulls/6864/
(reminds me to finally complete my IRC-gitian-build-bot and collect some devs pub keys to allow dev-build-sig-checks)

@jonasschnelli
Copy link
Member

jonasschnelli commented Oct 22, 2015

Ubuntu/Linux HiDPI looks ugly,.. but not related to this PR.
Fine for me:
bildschirmfoto 2015-10-22 um 21 27 29

Crash on OSX:

jonasschnelli$ ~/Desktop/bitcoin-qt --regtest
Assertion failed: (fixedFontInfo.fixedPitch()), function clear, file qt/rpcconsole.cpp, line 470.
@MarcoFalke
Copy link
Member Author

MarcoFalke commented Oct 22, 2015

To properly support HiDPI, I think we need to do the switch to qt5.

Windows works now, as well. (Courier New 10pt)

win-7e7d299

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:MarcoFalke-2015-qtMonospace branch 2 times, most recently Oct 23, 2015
@laanwj
Copy link
Member

laanwj commented Oct 23, 2015

To properly support HiDPI, I think we need to do the switch to qt5.

Supporting HiDPI is not a requirement for Qt4. All the distribution binaries are built with Qt5. We still support Qt4 in the same way as websites support IE6, e.g. bare-bones: functionality must work but it is allowed to be ugly.

@luke-jr
Copy link
Member

luke-jr commented Oct 23, 2015

Uh, Qt4 should remain a fully-supported platform until KDE has finished switching away from it... (this case would be an exception IMO since Qt4 itself lacks HiDPI support.)

@fanquake
Copy link
Member

fanquake commented Oct 24, 2015

@jonasschnelli Your OS X binaries are crashing for me, are you having any issue with them?

@jonasschnelli
Copy link
Member

jonasschnelli commented Oct 24, 2015

@fanquake: the crash is related to the PR. Monospace is not available on Mac (or something similar).

@jonasschnelli
Copy link
Member

jonasschnelli commented Oct 24, 2015

@fanquake: you probably picked an old build. Always check the git tip. Just re-started the build of the PR.

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:MarcoFalke-2015-qtMonospace branch Oct 24, 2015
@MarcoFalke
Copy link
Member Author

MarcoFalke commented Oct 24, 2015

This is ready for review now.

@fanquake and reviewers: Please use jonasschnelli's binaries for debug (send me stdout when no monospace font is found).

The code in this PR has the debug to stdout removed in 1d15e0e762cdfa83d4f98d96e532e35c0f8f6cf4 , was squashed and force pushed.

@dexX7
Copy link
Contributor

dexX7 commented Oct 24, 2015

On the first glimpse (general UI, address manager, debug console) it looks good on Win10:

mono

@jonasschnelli
Copy link
Member

jonasschnelli commented Oct 26, 2015

I don't have a fixed space font on OSX. Also the font is relatively small now.

This PR:
bildschirmfoto 2015-10-26 um 17 16 46

Current ~Master:
bildschirmfoto 2015-10-26 um 09 26 24

@dexX7
Copy link
Contributor

dexX7 commented Oct 26, 2015

Also the font is relatively small now.

In this case I wouldn't further increase the size, because then it probably gets too big on the other OS, but I agree, it looks too small.

While this doesn't seem to be an issue with fixed space font (especially since it's not available on OS X, as it seems), it may be worth to consider using a relative size based on a common, well sized font as reference, i.e. instead of using 10pt:

  1. retrieve actual font size of "reference font"
  2. use the reference size as size for the console font (whatever font it is)
@MarcoFalke MarcoFalke force-pushed the MarcoFalke:MarcoFalke-2015-qtMonospace branch Oct 27, 2015
@MarcoFalke
Copy link
Member Author

MarcoFalke commented Oct 27, 2015

@dexX7 If there was a "reference font", you could as well just not set the pt size at all, I guess. Let's try this?

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:MarcoFalke-2015-qtMonospace branch Oct 27, 2015
@MarcoFalke
Copy link
Member Author

MarcoFalke commented Oct 29, 2015

@jonasschnelli Lucida Grande can be monospace. According to your screenshot it also looks monospace but I am not sure why QT does not detect it as such...


For reference the binaries were built with eb475c0.

I removed the logging again and force pushed a squash.

@MarcoFalke
Copy link
Member Author

MarcoFalke commented Oct 29, 2015

If no one complains, this should be merged.

More screenshots:

Windows

  • nightly
    nigh
  • this PR
    mono

Fedora

  • nightly
    screenshot from 2015-10-29 17-46-39
  • this PR
    screenshot from 2015-10-29 17-45-51

Fedora

@laanwj
Copy link
Member

laanwj commented Nov 2, 2015

Still has a crazily big font on Ubuntu. Tending towards closing this, it's not worth the hassle.

@MarcoFalke
Copy link
Member Author

MarcoFalke commented Nov 2, 2015

@laanwj Unfortunately I can not reproduce the issue you mentioned. I did a fresh install of Ubuntu 15.10 and got the results below. Also, @dexX7 could not find an issue for Ubuntu. Maybe you changed the default settings for the system fonts accidentally?

$ gsettings list-recursively  org.gnome.desktop.interface|egrep "(font|text-scaling)"
org.gnome.desktop.interface monospace-font-name 'Ubuntu Mono 13'
org.gnome.desktop.interface text-scaling-factor 1.0
org.gnome.desktop.interface font-name 'Ubuntu 11'
org.gnome.desktop.interface document-font-name 'Sans 11'

Ubuntu

  • Nightly

nigh-ubu

  • This PR

mono-ubu

OSX

(screenshots from @jonasschnelli)

  • Nightly

76316bc0-7c05-11e5-8124-bd25c9cc3dec

- This PR

88fc0cb0-7e44-11e5-916a-c8b1409f4811

@laanwj
Copy link
Member

laanwj commented Nov 2, 2015

Looks like it's the same:

$ gsettings list-recursively  org.gnome.desktop.interface|egrep "(font|text-scaling)"
org.gnome.desktop.interface monospace-font-name 'Ubuntu Mono 13'
org.gnome.desktop.interface text-scaling-factor 1.0
org.gnome.desktop.interface font-name 'Ubuntu 11'
org.gnome.desktop.interface document-font-name 'Sans 11'

Have tried it on two systems. I'm using Ubuntu 14.04 and Qt5. I compile using the system's Qt5 library and am not using depends.
It is curious that 15.10 does display it in a normal font so maybe it's an Ubuntu issue that was fixed but not in LTS.

Edit: tried with Qt4, same problem.
I'll try with a 15.10 VM.

@laanwj
Copy link
Member

laanwj commented Nov 2, 2015

15.10 - fresh install. What am I doing wrong?

Before:
untitled

After:
screenshot from 2015-11-02 16 46 33

@MarcoFalke
Copy link
Member Author

MarcoFalke commented Nov 2, 2015

@laanwj Thank you for trying this. Would you mind to edit your posts to include the screenshots for the build without this PR?

@laanwj
Copy link
Member

laanwj commented Nov 2, 2015

Added for the 15.10 VM above: for the others the effect is exactly the same, except for the code changes as they were at the beginning of this pull request, there the fixed and proportional font were almost the same size.

@MarcoFalke
Copy link
Member Author

MarcoFalke commented Nov 2, 2015

[...] beginning of this pull request, there the fixed and proportional font were almost the same size.

Yes, it was "hard-coded" at 10pt but @jonasschnelli complained 10pt won't work in OS X, IIRC.

@laanwj
Copy link
Member

laanwj commented Nov 3, 2015

So it looks like Ubuntu consistently specifies a huge monospace default font. I guess that's simply the Ubuntu Mono 13 from qsettings? That's kind of annoying. I just never noticed on other programs - there are only few programs that use monospace besides the terminal, which seems to choose its own.

The only thing I don't understand is that you don't get the same.

@jonasschnelli
Copy link
Member

jonasschnelli commented Nov 3, 2015

Tested ACK.

Ubuntu:
bildschirmfoto 2015-11-03 um 10 05 54
bildschirmfoto 2015-11-03 um 10 05 44

bildschirmfoto 2015-11-03 um 10 08 58

Ubuntu HiDPI:
bildschirmfoto 2015-11-03 um 10 03 39

OSX:
bildschirmfoto 2015-11-03 um 10 04 40

@jonasschnelli
Copy link
Member

jonasschnelli commented Nov 3, 2015

Maybe someone can test this over a Qt4 build? (@luke-jr)?

@laanwj
Copy link
Member

laanwj commented Nov 3, 2015

I've already tested Qt4. Works OK.

@MarcoFalke
Copy link
Member Author

MarcoFalke commented Nov 3, 2015

I guess that's simply the Ubuntu Mono 13 from qsettings?

No, qt selects "DejaVu Sans Mono 12", and I set the size to QFontInfo(QFont()).pointSize(), which is less than 12pt (e.g. 10pt or 9pt) but equal to the font height we had before for the rpc console.

the terminal, which seems to choose its own.

No, terminal chooses the default "Ubuntu Mono 13".

The only thing I don't understand is that you don't get the same.

Maybe, there is a difference when you use the gitian builds by jonasschnelli ( 9pt) and your own build (10pt). Probably, because gitian uses qt5 and you are using qt4?

@laanwj The monospace font appears larger than the non monospace font because it consumes way more horizontal space. The only thing I can do here is to scale the font's height by a hard coded factor (like ~95%).

I have added a commit to do just that. If a constant factor can't fix your concern (exact value up for bike shedding), you are welcome to close this PR.

@laanwj
Copy link
Member

laanwj commented Nov 3, 2015

Maybe, there is a difference when you use the gitian builds by jonasschnelli ( 9pt)

Right - the gitian build by jonasschelli uses static qt from the depends system, I build against the system's Qt5 library (this is what the Ubuntu PPA does).
Fine with a fixed factor, seems an Ubuntu problem, don't want to bikeshed this further either.

@MarcoFalke
Copy link
Member Author

MarcoFalke commented Nov 3, 2015

As OS X was the only one getting it right, I expect the constant factor makes the font look smaller now on OS X. Hopefully such that it is still acceptable.

@jonasschnelli
Copy link
Member

jonasschnelli commented Nov 3, 2015

I think this is a improvement over the current version and the PR is pretty clean. Minor font size differences are an upstream issue IMO and still result in a adequate quality. Also the indent formatting increase readability.

@MarcoFalke
Copy link
Member Author

MarcoFalke commented Nov 3, 2015

@jonasschnelli So you are against 268b79e?

@jonasschnelli
Copy link
Member

jonasschnelli commented Nov 3, 2015

ACK 268b79e

@@ -462,13 +462,19 @@ void RPCConsole::clear()
}

// Set default style sheet
QFontInfo fixedFontInfo(GUIUtil::fixedPitchFont());
// Try to make fixed font adequately large on different OS
QString ptSize = QString("%1pt").arg(QFontInfo(QFont()).pointSize() * 8.5 / 9);

This comment has been minimized.

Copy link
@luke-jr

luke-jr Nov 3, 2015

Member

This seems weird. Why is this not simply using fixedFontInfo.pointSize()?

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Nov 3, 2015

Member

I also preferred to non-* 8.5 / 9 solution. But i have ACKed this solution because it might slightly reduce the size of mostly-oversized monospace fonts. But a *0.95 instead a * 8.5/9 would be cleaner, right.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Nov 3, 2015

Author Member

@luke-jr fixedFontInfo.pointSize() is 12, so way too large. Also, people were requesting that the fixed pitch font is exactly the same size as the previous default font.

@jonasschnelli QFontInfo(QFont()).pointSize() is 9, often. So we get a 8.5pt in this case instead of a longer float.

This comment has been minimized.

Copy link
@luke-jr

luke-jr Nov 3, 2015

Member

@MarcoFalke If fixedFontInfo.pointSize() is wrong, then that should be fixed instead. If people merely don't have their systems configured correctly, that isn't a problem we should try to workaround in the application.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Nov 4, 2015

Author Member

@luke-jr Maybe you could do something like http://stackoverflow.com/a/12179548 to read the system default font. But I am not planning to do this within this PR, as this will likely change the text for the whole GUI.

I see only three options for this PR:

  • Close
  • Merge 28313b8 (Monospace with "old" text-height)
  • Merge 268b79e (Same as above with constant scale factor)
@luke-jr
Copy link
Member

luke-jr commented Nov 3, 2015

268b79e looks fine to me, but it can be improved by using the fixed font for the input as well. This nit should at least get a comment explaining the rationale. Also, next time, please do the rename (bitcoinAddressFont -> fixedPitchFont) in a dedicated commit.

@laanwj
Copy link
Member

laanwj commented Nov 4, 2015

This is already bikeshedded to death, please let's not start another round.
I personally couldn't care less if the console font is monospace or proportional but some people want this and it seems to work acceptably, so I'm going to merge it now.

@laanwj laanwj merged commit 268b79e into bitcoin:master Nov 4, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Nov 4, 2015
268b79e [qt] rpcconsole: Scale monospace font to 95% (MarcoFalke)
28313b8 [qt] Use fixed pitch font for the rpc console (MarcoFalke)
@MarcoFalke MarcoFalke deleted the MarcoFalke:MarcoFalke-2015-qtMonospace branch Nov 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.