FIX(client): Use correct font size for scaling status icons #5822
Conversation
|
@Krzmbrzl This should be it, but I need to test this for a broken platform Currently waiting for the windows build to try in an old win7 VM which suffers from the problem |
|
1.4.274 (build from website) 1.5.0 (build from pipeline) No way for me to test this in the 1.4.x branch due to OpenSSL3 and not having a windows environment. I think this should work though. I will now re-push the commit with a proper message and then it is good to go from my end. |
There was a problem hiding this comment.
Alternatively, we could try to use the font of QTreeView, but I do not know if the font values are correct in its constructor.
My go-to strategy for these cases: Init the variables with the font size of the font returned by font() inside the constructor (typically too small) and then add a QTimer::singleShot(0, ...) in which you re-calculate the font size again in the next iteration of the event loop. By that time, the font size usually has updated as expected.
In theory I would agree, but I am not sure if the QTreeView font object has any influence what-so-ever on the UserDelegate objects paint routine. As far as I can tell the QStyledItemDelegate may choose to use a font from a theme or stylesheet or whatever without ever using or falling back to the QTreeView font. I might be wrong, though. |
Indeed. But I would argue that if we did this procedure in the |
Aha! There is the next problem! The |
|
I love Qt |
|
Oh ffs 🙄 |
|
In that case, let's test whether the font size of |
Ok. this would essentially roll back this MR and just change |
Yes - plus the trick with |
Hartmnt
force-pushed
the
fix_icon_size
branch
2 times, most recently
from
0e513a3 to
4d2a871
Compare
|
@Krzmbrzl Good news and bad news! Good news: The initial TreeView font size is wrong, but the size after QTimer seems to be correct. So you were right about this. I have used a testing time of 5 seconds in QTimer to be able to tell the difference. Bad news: The icon size does not update automatically, yet. Only when you hover over the tree view after the QTimer has run. I will try to find a method to forcefully update the treeview. I am reasonably confident that Qt has a function for this, but if this does not work, we cant use this method. |
Hartmnt
force-pushed
the
fix_icon_size
branch
4 times, most recently
from
64beb2e to
315322c
Compare
|
@Krzmbrzl Kindly check out the new commits. It works now using The second new commit changes the pointer to a qt_unique_ptr for 1.5 If you like the second approach more, I will squash the original one. |
|
@Krzmbrzl Did you have the chance to look at the latest version of the fix, yet? Also: Do you think this and the chat log issues will require a new hotfix release? |
|
Not yet, sorry.
Yeah, I guess that would make sense 🤔 |
There was a problem hiding this comment.
Now the code looks solid to me 👍
I'm only wondering: As we are touching the code already, maybe it would be time to change the naming from "flag" to a more generic "icon"? E.g. adjustFlag -> adjustIconSize (and also rename those variables)?
The second new commit changes the pointer to a qt_unique_ptr for 1.5
If you like the second approach more, I will squash the original one.
I do prefer that approach, but let's keep it in its separate commit. That way we can backport only the actual fix and leave the refactoring in the 1.5 branch.
I was referring to using |
Ah okay - Yeah, the new approach is much, much better imo 👍
Perfect :) |
|
Added your changes and both commits compile |
|
Did you re-check that the most recent approach still fixes the issue we saw on Windows? (just to be sure) |
No, but to do this I have to wait for the pipeline since i have no windows environment. I will let you know |
|
Prefect - in that case I think we can merge this as-is. Objections? |
Objection: Looking over the code I found that the comment you added still refers to adjustFlag. I am gonna change that real quick. |
Previously (mumble-voip#5772), we implemented a fix to set the status icon size according to the user display scaling. The calculation is based upon font sizes. However, due to an oversight, the wrong font size was used as the base for this calculation. However, the problem was not visible, if by coincidence the correct font size was the same as the one used by accident. This commit changes the calculation of the icon size once again to use the correct font information. Fixes one part of mumble-voip#5817
|
Ok seems good to me. Better wait for the checks just to be safe. |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
This merge requests fixes an oversight in #5772 on my part
Fixes one of the issues mentioned in #5817
I misunderstood the code at hand a little. Especially, I introduced this line. It was supposed to use the font used by the TreeView to determine the icon size. However, I now realize
prefers to the MainWindow and not the TreeView (UserView INHERITS TreeView), so it was using the wrong font metric.Why was this not caught?
This is coincidentally related to the log text issues #5820. As far as I can tell, the log text uses the exact same font object I was basing my calculations of.
For SOME platforms/Qt the Log text size was decreased for reasons that are out of scope of the problem hat hand. All platforms that I used for testing (Ubuntu Linux with XFCE and GNOME) did not have the log font size decrease problem (see screenshots of original MR).
That means when the log text had the same font size as the TreeView, this problem was not visible.
Solution:
In this MR the icon calculation is moved to UserDelegate's paint function. Which is admittedly slightly ugly, but the only way to implement this properly I think. Alternatively, we could try to use the font of QTreeView, but I do not know if the font values are correct in its constructor.
TODO: