High/Low DPI Adapting

[Cryspia]

The Qt UI would change the size according to the logical DPI of the computer Only Windows and Linux have been tested. (Should also work on Mac but have not tested). The function is unfinished.

Fixes #576

[Cryspia]

Plan:

• [x] Build framework for this function.
• [x] Implement the framework on every UI.
• [x] Fix the logo problem.
• [x] Finish the mixin for the class.
• [x] Write unit test
• [x] Set column width for talkeditor

[Cryspia]

Problems/Questions:

• The Logo on About Page is broken. I cannot find where to move it.
• Not very sure about how to construct the framework. Presently, I plan to write a mid layer class of all the classes that been used in QtGui. The Widget class will inherit from those classes instead of directly from QtGui classes. So that after I rewrite the methods like 'setFixedSize' and 'setMinimumSize', the Widgets do not need big changes. But personally, I think it not good idea because a lot classes and methods need to be rewrite, it is no better than change the Widgets one by one.

[Cryspia]

576

[dideler]

The logo was recently modified in #603 and it was a pain to get right. Let's worry about that later.

Looking at your current code, I see the following repeated for every widget:

STANDARD_DPI = 96.0
X_SIZE = self.logicalDpiX() / STANDARD_DPI
Y_SIZE = self.logicalDpiY() / STANDARD_DPI

Is that the part you plan on putting in the new parent class?

[Cryspia]

@dideler Yes. And I want to build the new class so that I do not need to repeat it every time.

[Cryspia]

Add the interlayer QT classes for the widget classes that transmit fixed width and height value to DPI dependent ones.

Personal, I feel like the method a bit ugly. So I need suggestions on how to improve it.

[zxiiro]

Please follow our guidelines on how to do proper docstrings.

[zxiiro]

I'm not an expert on DPI but I'm a bit surprised Qt isn't aware of it. Anyway the method looks fine for me but maybe other mentors have better ideas.

[Cryspia]

@zxiiro I am not very sure whether the docstrings you mentioned is the file docstring or the class/method docstring, so I changed the file docstring only in the update.

It seems that QtCore.Qt.WindowFlags cannot be import by using

from PyQt4.QtCore.Qt import WindowFlags

The error message says that QtCore has not Qt component But I can correctly use it by using

QtCore.Qt.WindowsFlags

[zxiiro]

I mean the class and method docstrings. A bunch of

'''
Constructor
'''

Does not explain anything and also doesn't follow the standard style of:

'''Short summary
Followed by
possibly many lines
to describe the function
'''

[zxiiro]

For the Qt file we only need to go as far as:

from PyQt4.QtCore import Qt

We do not need to go any deeper than 1 level after the QtCore/QtGui module.

Edit: if you need an example of the import style I want take a look at some of our new Qt GUI files such as https://github.com/Freeseer/freeseer/blob/master/src/freeseer/frontend/qtcommon/FreeseerApp.py

[Cryspia]

I found that for some methods, the input arguments can be either (width, height) or (QSize). So I add a new method to deal with the two situations.

[Cryspia]

@mtomwing I replace the global variable with a class. Does that fit the style now?

[mtomwing]

No.

[Cryspia]

@mtomwing I have two plans to solve the global dpi problem.

• make the classes that do not have logicialDpi as methods of QWidget (for example QWidget.QSpaceItemWithDpi()). So they can using the inherited logicialDpi.
• Simply pass logicialDpi as arguments into those methods. Which one do you think is better.

[Cryspia]

The new version has been pushed. In order to remove the global variable, I load the QSpacerItem and QRect into QWidget and QMainWindow as methods. Not sure whether it is OK. I think I have included all the widgets and windows which are relevant to this change, but there still could be something missing. I anyone find some, please tell me.

[Cryspia]

Should I write a unit test for this file?

[dideler]

@Promm how would I manually test these changes?

Also, can you move "Related #..." from the PR title to the PR description? The reference isn't useful in the title, since GitHub doesn't turn it into a reference there. And is this PR related or does it close that issue?

[dideler]

I'm okay with tests for this, unless the other mentors think you could better spend your time on something else.

[Cryspia]

@dideler I used Related because this PR is not finished yet, but it seems that WIP is better. After merged, this PR will completely fix that issue. To change the DPI, it depends on the desktop environment you are using for your linux. I am using Xfce4, and the DPI can be changed in settings->appearance->fonts->dpi. For Unity, it seems to be a SeekBar in the resolution settings. I am not familiar with other desktop environments so I may not be able to help you if you are using others.

[dideler]

Looks like changing DPI is overly difficult to do properly in Ubuntu (the slider doesn't change DPI for everything, just menus and titles), so I won't attempt it. Can you post before and after screenshots of your fix?

[Cryspia]

@dideler Here are the screenshots: Before this PR:

After this PR:

[Cryspia]

There is still one problem: In the talk editor window, I cannot find where to adjust the column width of the table (like I did in log window). That is why the contents in talk editor table are still crowded.

[dideler]

Thanks Kaiyuan, looks awesome.

[mtomwing]

It looks like there's a lot of duplicate code within the enhanced DPI classes. What about using a mixin (i.e. multiple inheritance) to store the common logic?

I'm also not sure why there are two super calls in most of the methods.

[Cryspia]

@mtomwing For the two super calls in one method, it is a idea from @dideler . We use that as a filter of typeerrors.

I will check whether I can improve the code using mixin.

[mtomwing]

It seems very weird that the same method is being called twice in succession. To me that indicates that the method can result in or do multiple things depending how/when it is run, which seems bad.

[Cryspia]

I have tried the mixin, but seems that there are some problems.

1. The resize() method and the setFixedSize() method are both in QWidget class, so we can consider that they are the same structurally. But they act differently in mixin struct. For resize(), I have to put it into the base class (I used QWidgetWithDpi class as the base), otherwise the resize will be done multiple times (multiples with the dpi rate every time) and the window will capture the whole screen finally. But for setFixedSize(), I cannot put it into the base class, or it will call itself recursively and overflow the system stack. What is the reason for that difference?
2. Actually the mixin GUI result is a bit different from non-mixin version: Non-mixin: mixin: The mixin version has larger height than the non-mixin version. I changed nothing in the code except for the mixin things, and there is nowhere in the related code that only change the height. So I totally have not idea how could this happen.

[Cryspia]

Maybe the mixin for overlapped classes will cause this unpredictable behavior? Or maybe it is predictable but I need to do something special to fit the pattern?

[mtomwing]

Well you probably only want to put the common logic inside the mixin. You can lookup "python MRO" to read more about how Python picks which parent method to use.

[dideler]

@mtomwing fyi, here's the discussion that lead to the awkward double calling.

[Cryspia]

I have tried the mixin with no class methods, but still got the resolution problem that I mentioned above (The config window height for mixin is larger than non-mixin). I found it can be a problem about QWidget.setMinimumSize() method. Because only the config window uses that method, and only the config window has that resolution problem. What is more, I found that changing that method will get unpredictable results. I will comment on that code with details.

[Cryspia]

@dideler Although some problems about the mixin problems are still unsolved, I plan to start the unit test writing first. I have some questions about the monkeypatch and setup/teardown combination. Can I use monkeypatch in the setup/teardown process? I cannot find any reference for this.

[dideler]

In pytest you can either use the classical setup/teardown or fixtures.

You can monkeypatch in setup/teardown or fixtures.

[Cryspia]

@dideler Do you have any ideas about the problem I mentioned above about the mixin?

[mtomwing]

My best guess (without trying to debug it myself) is that there's some weird interaction with the repeated method calls and the mixin. Might have time to look into it later.

[mtomwing]

You should check the DPI values before/after the mixin and see how much they differ by too. That might hint as to where the problem is.

[Cryspia]

@mtomwing The situation is very strange. I add a print in the setMinimumSize() and in the adjust_dpi(). And I found that in the input arguments are different. For the setMinimumSize(), The input arguments are (800,400), but in the adjust_dpi(), they becomes (800, 480).

The arguments should be passed directly from setMinimumSize() to adjust_dpi without any changes made. And the number "480" has never appeared in the previous code. How could this happen?

[Cryspia]

The unit test has been added.

The build failed just because of the setMinimumSize() Does not act correctly. The details can be seen in the build error. In the unit test, whatever values that is smaller than (480, 480) I pass to setMinimumSize(), the final size all always be (960, 600)

[Cryspia]

I am thinking about if I can replace setMinimumSize() with resize() in the source code so that I do not need to deal with this strange problem any more.

[zxiiro]

@Promm Can you explain to me what you are expecting setMinimumSize() to do when you call QWidget.setMinimumSize()?

[Cryspia]

The bug has been solved. Thanks for all your helps!

[dideler]

@Promm what's the status of this PR? Is it good to merge on your end?

[Cryspia]

@dideler Not yet. I want to make some changes with the talkeditor to define column width of the table. So that the problem I mentioned under the screen shots can be solved.

[Cryspia]

OK. The column setting for talk editor is also finished. The branch is ready to merge. Here is the screenshot for the talk editor now: The titles in the table can be show in completely.

[dideler]

Thanks. I'll review soon and merge if it's all good.

[Cryspia]

@dideler I have rewritten the comments you mentioned in the code review, but I am not sure they meet your requirement. If some of them still have problems, just comment them again.

[Cryspia]

@dideler Comments updated

[dideler]

Thanks for the contribution @Promm!