PCGen / pcgen

Main code and data development for pcgen program release
http://pcgen.org
GNU Lesser General Public License v2.1
431 stars 341 forks source link

Bad user experience with high DPI and dark theme #1760

Closed grebenyukaa closed 5 years ago

grebenyukaa commented 7 years ago

I use GTK+ look and feel option to make pcgen support DPI 200 with java 8 on arch linux. Everything seems to be in order except the list box control, which is used, for example, for listing the inventory or skills. See attached screenshot for further explaination. 204611_02462016

Thanks in advance!

LegacyKing commented 7 years ago

@cpmeister is this a theme we supply?

cpmeister commented 7 years ago

No really, but this Look and Feel is native to the linux os family. PCGen wasn't designed to look nice in GTK+ due to various limitations. PCGen was designed work well with the Nimbus or Windows Look and feel. Trying to make PCGen look nice in all Look and Feels causes more headache than it is worth so we instead focused on ones that are packaged with the JRE.

abbradar commented 7 years ago

Unfortunately, Nimbus doesn't support HiDPI monitors so it's very inconvenient (if not impossible) to use PCGen on them with this LaF. FWIW GTK LaF is packaged with JRE and is used automatically on GNOME Linux desktops (and, for example, by PyCharm). It's also the only one which supports HiDPI on Linux (with default widgets), So I'd be very nice to fix at least several usability problems:

This should assure basic usability on HiDPI monitors and dark themes, I think. Notice that I don't have knowledge in Java GUI programming so this very likely may be more tricky than it sounds above...

EDIT: added an issue with font size in some list boxes. I'll provide screenshots later when I have my monitor nearby.

abbradar commented 7 years ago

screenshot_2016-10-04_13-26-32 screenshot_2016-10-04_13-26-15 screenshot_2016-10-04_13-25-36 screenshot_2016-10-04_13-23-57 screenshot_2016-10-04_13-23-33

As you can see, it's mostly cases of not using system defaults for font sizes and to calculate heights of fields. Also icons are small (which is understandable because they are not vectorized and is a small issue). Apart from that PCGen actually looks okay on a high DPI monitor. I'm using a classic black-on-white theme so issues with using black font colors instead of system ones are not visible here.

LegacyKing commented 7 years ago

Just an update. Java FX is the only solution we've come up with. Re-writing the Java UI from Java Swing to JavaFX is underway, but stalled. Awaiting anyone with java experience to step forward and assist.

grebenyukaa commented 7 years ago

Well, I created a PR, that nicely fixes incorrect row heights via FontMetrics.getHeight(). There are some issues with colors, however.

grebenyukaa commented 7 years ago

I don't say that it's the best solution - maybe this rowHeight setting should be set in some baseclass of all used JTables, but I was unable to find any, so I just replaced every hardcoded row height value I could find with grep -r

grebenyukaa commented 7 years ago

@LegacyKing I also managed to find out, that the value for QUALIFIED_COLOR, which is used to color text in those table rows, which can also be red or blue, is loaded from the config file legacy.ini from the setting named prereqQualifyColor.

To my mind this is a serious maintainability issue, because there is stored a hex value of what is assumed to be the text color in a target OS. I think it should be stored as string "system" or smth like that, and the parser for this setting should be modified accordingly.

And moreover, if the system theme is chaged, this setting will not be altered accordingly and will remain to represent the system text color(if the PR is accepted) from the first run of PCGen, or just the hardcoded black color(as it is implemented in current master branch).

LegacyKing commented 7 years ago

I've pulled in these changes and these will be included in the 6.07.03 release.

LegacyKing commented 7 years ago

Hey @grebenyukaa having an issue where the stats table is completely condensed - rendering the spinboxes too small. Can you take a look at that?

LegacyKing commented 7 years ago

image

grebenyukaa commented 7 years ago

Okay, I'll look into it.

kheinzelmann commented 7 years ago

Is it possible to do something similar to what was done in the PostLevelUpDialog where the row height is based on the preferred size of the JSpinner added to the table?

There is also an issue with the CharacterHPDialog the fifth column has an IntegerEdit input. It actually took me a while to verify this since I have never actually used this dialog box. It shows up when you click the edit button next to your HP on the summary tab.

grimreaper commented 5 years ago

@kheinzelmann sorry about the delay. Fixing this is certainly possible, but we need more people clueful about how to build UIs. I'm happy to review patches.

LegacyKing commented 5 years ago

I think we can close this for now. We have some workarounds.