obs-studio
obs-studio copied to clipboard
UI: Add WhatsNew Feature for *nix systems
Description
Add support for "What's New" dialogue on macOS and Linux.
ToDo:
- [x] Test on Linux
- [x] Remove temporary test URL
- [x] mbedTLS based implementation for Linux
Motivation and Context
This feature is used to notify users about changes or important updates, and should be available on other platforms as well.
How Has This Been Tested?
Tested on a Mac.
Types of changes
- New feature (non-breaking change which adds functionality)
Checklist:
- [x] My code has been run through clang-format.
- [x] I have read the contributing document.
- [x] My code is not on the master branch.
- [x] The code has been tested.
- [x] All commit messages are properly formatted and commits squashed where appropriate.
- [x] I have included updates to all appropriate documentation.
Added a mbedtls based version and CMake adjustments for Linux. The mbedtls path was only checked on macOS so far however.
Now all that remains is some cleanup.
Verified working on Linux now (at least with Flatpak):
I believe this is now ready for review, the CMake changes are what I'd like to get a second opinion on the most.
That's a whole lot of code just to show a "What's new" page - what's the reason for the dependency on mbedTLS and shipping a public key in code(!) for this?
We'd also need to differentiate between "news" and actual update changelogs, the latter of which should appear in the Sparkle update notification instead.
That's a whole lot of code just to show a "What's new" page - what's the reason for the dependency on mbedTLS and shipping a public key in code(!) for this?
This is based on the Windows implementation, which contains the same signature verification: https://github.com/obsproject/obs-studio/blob/master/UI/win-update/win-update.cpp
I think @notr1ch said that this was basically to prevent someone compromising the OBS server from publishing a malicious entry that would get pushed to users.
On Linux it was a matter of either adding a dependency on something like libkcapi
(Linux Kernel Crypto API wrapper), or using MbedTLS which we already use for RTMPS, so that seemed like the least worst choice.
We'd also need to differentiate between "news" and actual update changelogs, the latter of which should appear in the Sparkle update notification instead.
"What's New" is only shown after an update to a new version, whereas the changelog in Sparkle or the Windows updater is shown before. It is generally used to highlight specific larger changes or notify users about important updates (e.g. Twitch API deprecation requiring an OBS update). We've also used it to advertise the Survey to users for example.
Edit: I'll also say that I'm intending to go over the code again tomorrow and trim it down a bit. It started off with just taking the Windows code and unixy-fying it, but that resulted in some stuff that's just way too verbose or unnecessarily split out into a separate function as it's only used in a single place. It could probably more heavily rely on Qt abstraction to handle simple things like reading/writing files to trim it down further.
FWIW we already ship OBSPublicDSAKey.pem on macOS for Sparkle (if it's enabled in CMake), so we could also ship another PEM as well (if it's not the same key).
Hardcoding the key does seems somewhat archaic, but I don't know if there's a good way to read a file at compile time (std::embed
seems to be stuck in limbo, and #embed
will only come with C23),
That may be a question for @jp9000 as to why this is the way it is and how we should address it.
Updated now to use SecItemImport
for Mac, as well as some switches to Qt utilities to replace some old functions.
Looks fine at a glance to me. If @notr1ch and @PatTheMav think the implementation is good enough for now and further improvements/changes can be done later, then we just need to decide when we actually want to merge this.
Updated now to use
SecItemImport
for Mac, as well as some switches to Qt utilities to replace some old functions.
I checked "OBSPublicDSAKey.pem" that we ship in the Resources subdirectory, and it's the same key. I'd rather use that key then a hardcoded one. Also the feature requires obs-browser to be present, so configuration should fail if ENABLE_BROWSER
is disabled for conflicting configuration parameters.
I don't feel particularly strongly about hardcoding the key either way, so unless there are any objections from R1CH or Jim I can look into loading it from the file. Although that may require additional modifications to CMake/build scripts to include the file in Linux distributions as well.
What's New is - weirdly - not guarded behind the browser being available on Windows, but it's true that it doesn't really make sense for it to be enabled if it isn't, so I'll update that.
Okay implemented the following changes:
- WhatsNew is now only enabled if browser panels are enabled
- On Windows/macOS it is implicitly enabled
- On Linux is has to be explicitly enabled since adds an additional dependency (MbedTLS) and we probably don't want third-party distributions to have it on by default
- Added updater public key to
UI/data/OBSPublicRSAKey.pem
- Public key is now loaded from disk (Windows implementation remains unchanged)
I checked "OBSPublicDSAKey.pem" that we ship in the Resources subdirectory, and it's the same key.
How did you check? Because they're clearly not the same key. The existing pem file also is a DSA key rather than RSA.
@tytan652 @GeorgesStavracas Opinions on making this on-by-default for Linux? For RTMPS we use an "auto" setting in case MbedTLS isn't available, so I'm thinking we could mirror this here unless we want package maintainers to have to opt-out explicitly.
Didn't give a deep review, but from a quick glance, I don't think there should be any build flags controlling this. I think it should always be enabled if a browser source and mbedtls are available.
Opinions on making this on-by-default for Linux? For RTMPS we use an "auto" setting in case MbedTLS isn't available, so I'm thinking we could mirror this here unless we want package maintainers to have to opt-out explicitly.
I do want package maintainers to have to opt-out explicitly so ON
by default. RTMPS may need to have a Linux only default change but it's out of scope.
If the maintainer already disabled the browser plugin, this feature should be disabled with it. Or if the packager don't want to link to mbedTLS at all, a flag is needed to disable the feature.
Alright, we'll go with enabled by default then. Thanks for your input!
Updated based on the latest review:
-
ENABLE_WHATSNEW
is now set as an option on Linux/internal cache on macOS/Windows when browser panels are available -
WHATSNEW_ENABLED
definition is now set when it is enabled - Linux builds have it enabled by default, will fail if MbedTLS is unavailable
- Fixed some ifdefs that were still using
_WIN32
Finally was able to test this (took a bit of futzing around with the config for it to actually show anything) and got a crash when closing the browser window.
Looks like it requires obs-browser to be updated to include https://github.com/obsproject/obs-browser/pull/374 before this can be merged.
EDIT: It's also unfortunate that users are greeted with an empty window at first (CEF showing a blank, white page) instead of waiting for CEF to having finished the page and then opening the Qt window, but I'm afraid that would require rewriting the feature itself.
FWIW, it takes so long to load for me that I can see some people closing it before actually reading the contents.
Finally was able to test this (took a bit of futzing around with the config for it to actually show anything) and got a crash when closing the browser window.
Looks like it requires obs-browser to be updated to include obsproject/obs-browser#374 before this can be merged.
EDIT: It's also unfortunate that users are greeted with an empty window at first (CEF showing a blank, white page) instead of waiting for CEF to having finished the page and then opening the Qt window, but I'm afraid that would require rewriting the feature itself.
FWIW, it takes so long to load for me that I can see some people closing it before actually reading the contents.
Yup since your last version is presumably already set to 27.2.4 and the "WhatsNew" dialog is for 27.2.4 it won't show unless you decrement the LastVersion*
or the InfoIncrement
entries from the config first.
Weirdly I never had any crashes on my system, and it also didn't take particularly long to load, but I also already had a browser source active in my scenes, so perhaps my CEF was already "warmed up"?
Looks like the submodule has already been updated to include https://github.com/obsproject/obs-browser/pull/374 in master.
Successfully built and tested on Ubuntu 22.04 running Xorg. What's New dialog pops up successfully. There seems to be a short UI hang for 1-2 seconds.