-
-
Notifications
You must be signed in to change notification settings - Fork 9k
frontend: Demangle ScreenshotObj and OBSBasic #12186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
f763a84 to
73bcdc7
Compare
73bcdc7 to
3b1f7a7
Compare
PatTheMav
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach looks about right to me - the screenshot object cares for its own work and emits a signal about it, so the main window can handle that event and update itself accordingly.
However I'm concerned about a few things:
- The UI related functionality is implemented in
ScreenshotObj'sdestructor. This is not an issue with the PR, but it somehow feels "wrong" to me. 😅 - But at least this allows the code the create a new instance of
ScreenshotObjand connect to thescreenshotTakenevent, as the event would only ever be emitted when the object is destructed (which should probably happen after the function finishes). - Shouldn't
OBSBasic"disconnect" from the signal in its callback to avoid dangling connections for every screenshot taken? What is the state of the pointer during that callback, given that the signal happens during the constructor?emitdoesn't block as far as I can tell.
Again, the signal-based approach is correct, it's the whole "emitting in the destructor" part that concerns me.
|
Nevermind, I get what you mean now, as the signal is in the destructor, and is non blocking, as far as we know. |
|
Please rebase. |
8a6b92d to
a4cb818
Compare
|
@Warchamp7 I did try to apply both locally and they do conflict. Do you still want to block this on #12067 ? |
This adds a signal to ScreenshotObj, so it doesn't have to directly call OBSBasic.
a4cb818 to
0e2602f
Compare
Description
This adds a signal to ScreenshotObj, so it doesn't have to directly call OBSBasic.
Motivation and Context
Better code.
How Has This Been Tested?
Took screenshots to make sure they still worked.
Types of changes
Checklist: