Scale pyrdp-player replays with window height #208
Conversation
…according to window height)
…s. Defaults to False
There was a problem hiding this comment.
This is a very cool PR. Thanks for taking the time to work on this!
My only critique is that you seem to be ignoring the concept of parent widgets in Qt. I have a feeling that if we are properly setting the widget chain (MainWindow -> ReplayWindow -> ReplayTab), then the onResize events from Qt would propagate throughout the widget tree and avoid all of the need for this observable code and manual event dispatching. I am not sure, but I know a lot of GUI frameworks work like that, so if you can test it, it would be nice and greatly simplify the code.
If having parent widgets introduces other bugs, I am willing to accept this implementation first and iron out the bugs/refactor to be more idiomatic Qt later down the road.
I'll try and test this today and report back. I didnt think the resize events would propagate, but havent tested |
Bonus: pixel-perfect window scaling instead of arbitrary constant
|
It should be a better implementation now. It doesnt use the parent concept, but it is way cleaner still. |
…aking another blank one. This removes the need to seek upon changing the scale and greatly improves rescaling performance.
There was a problem hiding this comment.
A very precise change that has an important user-visible impact. Good work!
You can ignore most of my comments, they are more meant as a "watch out for next time" rather than a "change this or I won't merge".
I'm going to merge this in the next few minutes.
Note that I can't apply to a diff section:
pyrdp/ui/qt.pyhas diverged from the rpdy days and should now carry both project's headers
| # installed properly. Do not re-order. | ||
| import asyncio # noqa | ||
| # installed properly. ***DO NOT RE-ORDER***. | ||
| import asyncio # noqa | ||
|
|
||
| from twisted.internet import asyncioreactor | ||
|
|
||
| asyncioreactor.install(asyncio.get_event_loop()) |
| @@ -44,41 +44,41 @@ def __init__(self, replay: Replay): | |||
| self.lastSeekTime = 0 | |||
| self.requestedSpeed = 1 | |||
| self.replay = replay | |||
| self.timer = Timer() | |||
There was a problem hiding this comment.
Were these changes really necessary?
There was a problem hiding this comment.
It was in my previous implementation. I forgot to completely revert this change after I changed the implementation, so no they were not necessary.
| import rle | ||
| from io import BytesIO | ||
|
|
||
| import rle |
There was a problem hiding this comment.
FYI: rle is not an stdlib module. Your import organizer is broken and you shouldn't have made that change.
| @@ -151,7 +163,6 @@ def runOnMainThread(self, target: callable): | |||
| @property | |||
| def screen(self): | |||
| return self._buffer | |||
|
|
|||
There was a problem hiding this comment.
Not sure what this is doing here given your taste to reindent and add new lines to things 😉
There was a problem hiding this comment.
No empty lines between a property and a method? This is PEP8?
There was a problem hiding this comment.
~I missed this one :( ~
There was a problem hiding this comment.
i'm now ashamed of myself
There was a problem hiding this comment.
haha it was mostly a joke
Works ONLY on replays, not on the live player tab (because I was too scared to commit to testing the whole session takeover feature to be bug free).
Right now, I will keep it as a draft because it is the default option, but I want to have a checkbox where we can choose to scale or not.EDIT: Checkbox has been implemented, images updated
Also maximizes pyrdp-player upon launch
Fixes #101