High/Low DPI Adapting #640

Cryspia · 2014-10-26T11:37:43Z

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 · 2014-10-26T11:41:19Z

Plan:

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

Cryspia · 2014-10-26T11:55:07Z

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 · 2014-10-26T13:01:59Z

#576

dideler · 2014-10-27T01:26:01Z

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 · 2014-10-27T01:58:07Z

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

Cryspia · 2014-10-31T10:01:38Z

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 · 2014-10-31T14:31:28Z

Please follow our guidelines on how to do proper docstrings.

zxiiro · 2014-10-31T14:31:48Z

src/freeseer/framework/dpi_adapt_qtgui.py

+'''
+freeseer - vga/presentation capture software
+
+Copyright (C) 2011  Free and Open Source Software Learning Centre


This is a new file so the year should be updated.

zxiiro · 2014-10-31T14:40:24Z

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 · 2014-10-31T21:12:18Z

@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 · 2014-10-31T22:25:59Z

src/freeseer/frontend/qtcommon/dpi_adapt_qtgui.py

+
+# freeseer - vga/presentation capture software
+#
+#  Copyright (C) 2011, 2014  Free and Open Source Software Learning Centre


This is a new file so only 2014 is enough

zxiiro · 2014-10-31T22:27:34Z

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 · 2014-10-31T22:29:22Z

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 · 2014-11-01T09:06:19Z

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.

mtomwing · 2014-11-02T23:40:22Z

src/freeseer/frontend/qtcommon/dpi_adapt_qtgui.py

+    the input value DPI adatping using LOGICAL_DPI. The output will
+    all be transfered to (QSize) type
+    '''
+    global LOGICAL_DPI


Global variables are typically frowned upon. Can you find a way to accomplish what you want without using them?

The only method that I can come up with is to generate a dummy QWidget class in other classes. But that could be low efficient. How do you think of that?

What about just computing this LOGICAL_DPI in each widget? There's no sense in preemptively optimizing before there's any problem.

What about make it a class member for QWidgetWithDpi and pass it as additional arguments to the QGroupBoxWithDpi and QSpacerItemWithDpi constructors?

Cryspia · 2014-11-04T02:04:42Z

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

mtomwing · 2014-11-04T02:56:41Z

No.

Cryspia · 2014-11-06T01:39:23Z

@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 · 2014-11-07T05:18:16Z

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.

dideler · 2014-11-07T10:42:25Z

src/freeseer/frontend/qtcommon/dpi_adapt_qtgui.py

+        '''
+        super(QToolButtonWithDpi, self).__init__(*args, **kwargs)
+
+        STANDARD_DPI = 96.0


How do we know that the standard DPI is 96? Is there a source somewhere that says this?

No. But both Windows and Ubuntu consider the standard DPI as 96 (I do not test mac because I do not have a mac). And quite a lot resources on the Internet also says that.

dideler · 2014-11-26T23:55:00Z

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

Cryspia · 2014-11-27T22:44:51Z

@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 · 2014-11-28T00:21:31Z

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 · 2014-11-28T00:25:24Z

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

dideler · 2014-11-28T23:44:26Z

src/freeseer/tests/frontend/qtcommon/test_dpi_adapt_qtgui.py

+
+    def test_set_width_with_dpi(self):
+        ''' The test cases to test set_height_with_dpi method '''
+        assert self.tested_cls.set_width_with_dpi(300) == 450


I think the tests would be more robust if they calculate the expected value using the ratio instead of hardcoding it.

Cryspia · 2014-11-30T03:36:07Z

@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.

dideler · 2014-11-30T20:18:53Z

src/freeseer/frontend/qtcommon/dpi_adapt_qtgui.py

+
+
+class QtGuiWithDpi(QWidget):
+    '''The base class uses for the mixin for other Dpi adapt QtGui classes.


This sentence is confusing, can you try to rewrite it?

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 unit test for the new file is also added.

Cryspia · 2014-12-02T03:27:11Z

@dideler
Comments updated

Make them more readable and informative. Also fix punctuation, spelling, and grammar mistakes. Related to #640

dideler · 2014-12-03T17:38:29Z

Thanks for the contribution @promm!

dideler added this to the 2014 Fall - UCOSP milestone Oct 26, 2014

Cryspia force-pushed the 576-dpi-adapt branch from 44b725a to 967d7cf Compare October 31, 2014 09:53

zxiiro reviewed Oct 31, 2014
View reviewed changes

Cryspia force-pushed the 576-dpi-adapt branch from f5244bf to 439a19f Compare October 31, 2014 21:06

zxiiro reviewed Oct 31, 2014
View reviewed changes

mtomwing reviewed Nov 2, 2014
View reviewed changes

Cryspia force-pushed the 576-dpi-adapt branch from 1ed4573 to 64bff1c Compare November 4, 2014 02:02

Cryspia force-pushed the 576-dpi-adapt branch from abf8871 to 5481d18 Compare November 7, 2014 04:18

dideler reviewed Nov 7, 2014
View reviewed changes

Cryspia force-pushed the 576-dpi-adapt branch from 3b8be2c to a59f816 Compare November 23, 2014 06:52

Cryspia force-pushed the 576-dpi-adapt branch from a59f816 to eec484d Compare November 28, 2014 00:15

dideler reviewed Nov 28, 2014
View reviewed changes

Cryspia force-pushed the 576-dpi-adapt branch from eec484d to d2390af Compare November 30, 2014 03:33

Cryspia force-pushed the 576-dpi-adapt branch from d2390af to 363ee9c Compare November 30, 2014 03:47

dideler reviewed Nov 30, 2014
View reviewed changes

Fixes Freeseer#576 High/Low DPI Adapting

c03a73b

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 unit test for the new file is also added.

Cryspia force-pushed the 576-dpi-adapt branch from 363ee9c to c03a73b Compare December 2, 2014 03:23

dideler merged commit c03a73b into Freeseer:master Dec 3, 2014

dideler added a commit that referenced this pull request Dec 3, 2014

📝 Improve docstrings

6a1330a

Make them more readable and informative. Also fix punctuation, spelling, and grammar mistakes. Related to #640

dideler mentioned this pull request Dec 9, 2014

Faulty DPI adapting #676

Open

Cryspia deleted the 576-dpi-adapt branch December 14, 2014 02:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

High/Low DPI Adapting #640

High/Low DPI Adapting #640

Cryspia commented Oct 26, 2014

Cryspia commented Oct 26, 2014

Cryspia commented Oct 26, 2014

Cryspia commented Oct 26, 2014

dideler commented Oct 27, 2014

Cryspia commented Oct 27, 2014

Cryspia commented Oct 31, 2014

zxiiro commented Oct 31, 2014

zxiiro Oct 31, 2014

zxiiro commented Oct 31, 2014

Cryspia commented Oct 31, 2014

zxiiro Oct 31, 2014

zxiiro commented Oct 31, 2014

zxiiro commented Oct 31, 2014

Cryspia commented Nov 1, 2014

mtomwing Nov 2, 2014

Cryspia Nov 3, 2014

mtomwing Nov 3, 2014

Cryspia Nov 3, 2014

Cryspia commented Nov 4, 2014

mtomwing commented Nov 4, 2014

Cryspia commented Nov 6, 2014

Cryspia commented Nov 7, 2014

dideler Nov 7, 2014

Cryspia Nov 7, 2014

dideler commented Nov 26, 2014

Cryspia commented Nov 27, 2014

Cryspia commented Nov 28, 2014

dideler commented Nov 28, 2014

dideler Nov 28, 2014

Cryspia commented Nov 30, 2014

dideler Nov 30, 2014

Cryspia commented Dec 2, 2014

dideler commented Dec 3, 2014



		class QtGuiWithDpi(QWidget):
		'''The base class uses for the mixin for other Dpi adapt QtGui classes.

High/Low DPI Adapting #640

High/Low DPI Adapting #640

Conversation

Cryspia commented Oct 26, 2014

Cryspia commented Oct 26, 2014

Cryspia commented Oct 26, 2014

Cryspia commented Oct 26, 2014

dideler commented Oct 27, 2014

Cryspia commented Oct 27, 2014

Cryspia commented Oct 31, 2014

zxiiro commented Oct 31, 2014

Choose a reason for hiding this comment

zxiiro commented Oct 31, 2014

Cryspia commented Oct 31, 2014

Choose a reason for hiding this comment

zxiiro commented Oct 31, 2014

zxiiro commented Oct 31, 2014

Cryspia commented Nov 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cryspia commented Nov 4, 2014

mtomwing commented Nov 4, 2014

Cryspia commented Nov 6, 2014

Cryspia commented Nov 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dideler commented Nov 26, 2014

Cryspia commented Nov 27, 2014

Cryspia commented Nov 28, 2014

dideler commented Nov 28, 2014

Choose a reason for hiding this comment

Cryspia commented Nov 30, 2014

Choose a reason for hiding this comment

Cryspia commented Dec 2, 2014

dideler commented Dec 3, 2014