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

gui: Install Inter variable font and basic stylesheet changes #1926

Closed

Conversation

opsinphark
Copy link
Contributor

@opsinphark opsinphark commented Oct 7, 2020

  • Install Inter-Variable font and remove constant versions
    Glyph Count:
    2537
    Variation Axes:
    Weight: 100 - 900, default 400
    Slant: -10 - 0, default 0
    Named Styles:
    Thin, Thin Italic, ExtraLight, ExtraLight Italic, Light, Light Italic, Regular, Italic, Medium, Medium Italic, SemiBold, SemiBold Italic, Bold, Bold Italic, ExtraBold, ExtraBold Italic, Black, Black Italic

  • Change default font-size to 11pt
    10pt is too small to display the font correctly.

  • Vertical spacing to 6 in the Overview Page

  • Lower font-weight to 550 on the labels in the Overview Page

BEFORE (font size 10pt, vertical spacing 10)
grc-before

AFTER (font size 11pt, vertical spacing 6)
grc-after

BEFORE (HiDPI) (font size 10pt, vertical spacing 10)
grc-before-hidpi

AFTER (HiDPI) (font size 11pt, vertical spacing 6)
grc-after-hidpi

 - This file includes all glyphs, variation axes and named styles
   where the constant files did not.  This should also cut down on
   final compilation size.
   Glyph Count:
     2537
   Variation Axes:
     Weight: 100 - 900, default 400
     Slant: -10 - 0, default 0
   Named Styles:
     Thin, Thin Italic, ExtraLight, ExtraLight Italic, Light,
     Light Italic, Regular, Italic, Medium, Medium Italic, SemiBold,
     SemiBold Italic, Bold, Bold Italic, ExtraBold, ExtraBold Italic,
     Black, Black Italic
Copy link
Member

@jamescowens jamescowens left a comment

Choose a reason for hiding this comment

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

I like it!

@opsinphark
Copy link
Contributor Author

Spacing could be better in the tables. I actually prefer one size smaller font size after looking at it for awhile.
Changing the spacing in the form file overviewpage.ui doesn't have any effect. I am wondering if something in the stylesheets is overriding it.
This is difficult to get right. Finding some middle ground here would be nice.

@opsinphark
Copy link
Contributor Author

opsinphark commented Oct 7, 2020

I encourage you to build it and test both modes (normal and hi-dpi).
I've noticed (on Ubuntu/Bionic) that going for normal display mode to hi-dpi the wallet had to be restarted to see the full effects. Mileage may vary here.

@opsinphark
Copy link
Contributor Author

I'd like to leave this PR open until several people have reviewed the effects.
I don't want to be the one responsible for making GRC look terrible

@opsinphark
Copy link
Contributor Author

I've change the horizontal and vertical spacing in the overview page form and updated the screenshots.

@jamescowens
Copy link
Member

I feel like the vertical spacing is too compressed now. Maybe a middle ground between the original and the new shots you presented. I also think we should try 11 points, between the original 10 and the 12.

@cyrossignol
Copy link
Member

cyrossignol commented Oct 9, 2020

Some initial thoughts...perhaps not entirely related to this PR...

I've seen reports that some people cannot resize the pre-Inter wallet small enough for some displays (Windows VPS, RDP sessions, custom single-board display kits, improperly configured VMs, who knows...). Perhaps those aren't our problems, but increasing the default font size may aggravate the issue.

Inter is highly-optimized for smaller font sizes. Legibility and visual appeal of body text may actually improve with smaller text and greater line heights because larger, compact text is difficult to scan (need to find the reference... something about it being harder for the eyes to lock into visual tracks).

Global font face and size properties in the stylesheets override the properties set manually on widgets via code or Qt designer. We will need to identify these items and move the definitions to the stylesheets if we keep the global properties.

We can't make everyone happy. Perhaps we can add a font-size option to the GUI settings in a later PR.

@cyrossignol
Copy link
Member

cyrossignol commented Oct 9, 2020

That said, I think I like the changes... need to stare at it for a while so that the newness wears off. Maybe the high-DPI version is a little squashed. I will wait to see what your upcoming commits look like too.

@jamescowens
Copy link
Member

BTW some user reported intersecting (overlapping) text. If you take the main window and force the width smaller, the columns in the transaction summary start to clash. We should insure that the minimum column widths (and initial widths) make sense. When I set up the peer tab, I had to scale the column width by logicaldpiX() / 96 to get them to come out close to right on low-res and high-res displays.

Would love to see thoughts on how to unmuck column size problems too.

Since we are in the engine room on this, we might as well clean it up...

@opsinphark
Copy link
Contributor Author

opsinphark commented Oct 9, 2020

BTW some user reported intersecting (overlapping) text. If you take the main window and force the width smaller, the columns in the transaction summary start to clash. We should insure that the minimum column widths (and initial widths) make sense. When I set up the peer tab, I had to scale the column width by logicaldpiX() / 96 to get them to come out close to right on low-res and high-res displays.

Would love to see thoughts on how to unmuck column size problems too.

Since we are in the engine room on this, we might as well clean it up...

For better scaling Qt DPI and HiDPI pixmap support need to be initialized in bitcoingui.cpp.

  • Qt Minimum version required is 5.6.x

#if QT_VERSION >= 0x050600
QApplication::setAttribute(Qt::AA_EnableHighDpiScaling); // DPI support
QApplication::setAttribute(Qt::AA_UseHighDpiPixmaps); // HiDPI pixmaps
qputenv("QT_AUTO_SCREEN_SCALE_FACTOR", "1");
//qputenv("QT_SCALE_FACTOR", "1");
#endif

I wasn't sure to include this or not. I wouldn't be able to test it properly so I haven't presented the change.

I think it is important we get the font size and form spacing to an agreeable size before deciding on form width/height constraints. I will make the changes @jamescowens requested regarding moving the font size to 11pt. and increasing the vertical spacing and we can go from there.

I've noticed making changes in Qt forms require you to make clean and recompile as the C++ headers the forms produce are not regenerated after being changed. This frustrated me for awhile so I thought I would pass it on.

…tical spacing

 - Default font-size: 11pt
 - Overview Page: Labels to font-weight: 550
 - Overview Page: Vertical spacing 6
@opsinphark
Copy link
Contributor Author

I've updated the default font size to 11pt. and vertical spacing on the overview page. Screenshots have been updated.
I'll start looking into the application geometry.

@jamescowens
Copy link
Member

Thanks. I will take a look over the weekend, and also play around with the hidpi settings. I have both non hidpi and hipdi monitors here, so can see how it looks in various situations.

@opsinphark
Copy link
Contributor Author

Overlapping text on the recent transactions on the overview page:

The option in the Gridcoin Wallet Settings -> Options -> Display - "Display addresses in transaction list" needs to be toggled off for those working in lower resolution environments.
grc-settings

Perhaps this option should be removed entirely for better consistency. This would make the default behaviour use one or the other, not both.
eg:
Current:
[LABEL or ADDRESS] [AMOUNT]
[LABEL] [ADDRESS] [AMOUNT] (with "Display addresses in transaction list" turned on)
^ mucks up the overview page as well as the transactions tab

Proposed:
[LABEL or ADDRESS] [AMOUNT]

@nathanielcwm
Copy link
Contributor

idk if I messed something up but the text looks off when I build off your branch

image

@lexieric
Copy link

At the moment if my wallet is in dark mode and I open the console it outputs all text and commands in white text on a white background (so you can only see it if you highlight it with the mouse). I am on mac if that helps.
image

@cyrossignol
Copy link
Member

cyrossignol commented Oct 17, 2020

The option in the Gridcoin Wallet Settings -> Options -> Display - "Display addresses in transaction list" needs to be toggled off for those working in lower resolution environments.
grc-settings

Perhaps this option should be removed entirely for better consistency. This would make the default behaviour use one or the other, not both.

I think this makes sense.

If we do change it, we may want to a toggle button on the transactions page that switches between addresses and labels for a future enhancement so that users don't need to jump through the options dialog when they need to see the flip side.

Or perhaps, instead of a yes/no option, someone can pick from "address", "label", or "both".

@opsinphark
Copy link
Contributor Author

After hearing some of the reactions I think the best approach is resetting the font changes and isolating the scope of this into one or two new themes that will be a work in progress. eg: WIP-Light and WIP-Dark.

Working with Qt and style changes in general isn't easy. One small change requires you to recompile the project from scratch to see the results. Changes will be slow and on going. With that being said making this into an opt-in change would give us some breathing room and wouldn't force any changes on the user as the original themes still remain.

The other big issue is that of application window size. The new mock up (https://projects.invisionapp.com/share/APJIDTNFV47#/screens) assumes everyone is running at high resolution, which is not the case. Deciding on a one that works for everyone is going to be another hurdle.

@jamescowens
Copy link
Member

I agree, except I don't want to undo the already merged selection of the new fonts. I am happy with that the way it is...

@jamescowens
Copy link
Member

The other challenge is that the op sys default styles on different platforms have different interactions with the wallet, and testing all of the combinations is exceedingly cumbersome.

@cyrossignol
Copy link
Member

I like the idea of WIP stylesheets. Perhaps "Testing - Light" and "Testing - Dark".

@nathanielcwm
Copy link
Contributor

Or maybe instead of a wip stylesheet there could be a legacy stylesheet?

This way the font changes won't need to be reverted in the next release.

@jamescowens
Copy link
Member

We are NOT reverting the use of the new fonts, which is already merged.

@nathanielcwm
Copy link
Contributor

Well the way I was seeing it with a WIP stylesheet is that it would be reverted and the changes would be placed into the WIP stylesheet.

I'm proposing to keep it as is but to also add a 'legacy' stylesheet which would contain the old stylesheet without the font changes.

@jamescowens
Copy link
Member

I am ok with a legacy stylesheet, but I do not want to go back to the wrong, proportional font for the console.

@opsinphark
Copy link
Contributor Author

My thoughts at this point were to remove forcing the "Inter" font in the stylesheet as default for everything in place and start from scratch with a new stylesheet that would include the changes. This would keep everything that's already merged in place as well as make it an opt-in change.
That part only requires removing the * {font-family: "Inter"; font-size: 11pt } from the stylesheets.

This reverts the default behaviour back to normal (using system fonts) except for the console which would use the included change.

I first proposed creating two new work in progress stylesheets. After looking at it, things would be much easier if we start with one until it's fully finished.

I haven't had much time to work on things these last few weeks but I would like to get this into a position where I can start fiddling with things without encroaching on other user's experiences.

@nathanielcwm
Copy link
Contributor

The console fonts are a very noticable improvement which afaik no one has any issues with so I agree with @opsinphark.

@jamescowens
Copy link
Member

I actually like the new fonts, and on the fireside we got positive feedback. I would say leave the new fonts in place and experiment with new stylesheets.

@jamescowens
Copy link
Member

I have implemented the hidpi scaling and done some other fixups related to scaling as part of #1978. Note that Windows does NOT react well to QCoreApplication::setAttribute(Qt::AA_EnableHighDpiScaling); so I put a define conditional around it.

@opsinphark
Copy link
Contributor Author

Closing. Solved in another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Low dpi error Squashed text after change to inter font.
5 participants