-
Notifications
You must be signed in to change notification settings - Fork 953
Expose settings dialog in coda #13937
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: distro/collabora/coda-25.04
Are you sure you want to change the base?
Expose settings dialog in coda #13937
Conversation
Signed-off-by: Shardul Vikram Singh <[email protected]> Change-Id: Ibce2d120ffd1675d2801a31538c91d733cccc617
Signed-off-by: Shardul Vikram Singh <[email protected]> Change-Id: I1d762dd8fed78aaab0094843a5e388e7aa52d2d3
Signed-off-by: Shardul Vikram Singh <[email protected]> Change-Id: I15e5dc65a5a7ee4b3e365d9d13ed1417ed6b09d3
Signed-off-by: Shardul Vikram Singh <[email protected]> Change-Id: I587db3002242356112f6541c3feda2f67fb29bbc
Signed-off-by: Shardul Vikram Singh <[email protected]> Change-Id: Iae5ca1724380cb823725206a46787941eaa04984
Signed-off-by: Shardul Vikram Singh <[email protected]> Change-Id: I8ea52d54a3e8316dfbbf0cbbc30840dcfbffbb8e
- This change replaces the redundant check-box svg logic with actual input check-box Change-Id: Ifd17fcea76770c1a6a27c6df2dac877a5134f915 Signed-off-by: Parth Raiyani <[email protected]>
Signed-off-by: Shardul Vikram Singh <[email protected]> Change-Id: Ia606c42771fab9fe8a09ceb888ee37e19f93ed5a
|
thanks for this PR! |
| COOL_IMAGES_CUSTOM_SRC = $(shell if test -n "$(CUSTOM_ICONS_DIRECTORY)"; then find $(CUSTOM_ICONS_DIRECTORY) -name '*.*'; fi) | ||
| COOL_IMAGES_CUSTOM_DST = $(patsubst $(CUSTOM_ICONS_DIRECTORY)/%,$(DIST_FOLDER)/images/%,$(COOL_IMAGES_CUSTOM_SRC)) | ||
| COOL_L10N_SRC = $(shell find $(srcdir)/l10n -name '*.json') | ||
| if !ENABLE_MOBILEAPP |
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.
This does not look good. In apps (mobile apps and desktop apps) l10n.js does not work. We have to bundle and load all translations at startup.
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.
Thanks @shardul for the PR. However, there are a couple of changes needed.
Current implementation observations:
-
Currently, all settings files are fetched and uploaded only via the settings iframe. This prevents the settings from being applied for the current session in
coolwsd. To apply these changes immediately, we need to load the presets for the relevant settings (code reference: see how this is handled for WOPI inDocumentBroker.cpp::PresetsInstallTask::install). -
Since these settings need to be supported across all platforms, it would be better to handle them via a new class under
/common. This would allow us to reuse the implementation across platforms almost for free once it’s done for one platform. -
When a setting is applied from the options (especially from the File tab backstage), it should take effect immediately. This should be addressed by the first point above. (Session sync problem, not sure possible for all setting file. cc: @caolanm )
-
I would suggest starting with browser-only settings (which are the most in demand) and making that fully functional first. Adding other sections afterward would be much easier.
| #include <Poco/Exception.h> | ||
| #include <config.h> | ||
| #include "bridge.hpp" | ||
| #include "COOLWSD.hpp" |
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.
build error: Missing #include <Poco/Environment.h>
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.
fixed
qt/coda-qt.cpp
Outdated
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. | ||
| */ | ||
|
|
||
| #include <Poco/Exception.h> |
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.
can we use std::exception?
| window.wopiSettingBaseUrl = element.dataset.wopiSettingBaseUrl; | ||
| window.iframeType = element.dataset.iframeType; | ||
| window.cssVars = element.dataset.cssVars; | ||
| // window.cssVars = element.dataset.cssVars; |
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.
Do we have any reason to hide this here? It would stop applying branding for other integrator styles.
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.
I removed it because adminintegratorsettings.html was not being preprocessed, which caused an error. I have added preprocessing in coda now, so this is uncommented again
| xcu: null, | ||
| }; | ||
| if ((window.parent as any).ThisIsAMobileApp) { | ||
| this.populateSharedConfigUI(data); |
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.
Why we try to populate data with every sensible filed null?
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.
ConfigData contains URLs of files on the WOPI server, so in the case of CODesktop it is null
| const image = document.createElement('img'); | ||
| image.src = `${window.serviceRoot}/browser/${window.versionHash}/admin/images/${imageSrc}`; | ||
| let src = `${window.serviceRoot}/browser/${window.versionHash}/admin/images/${imageSrc}`; | ||
| if ((window.parent as any).ThisIsAMobileApp) |
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.
I think ThisIsAMobileApp is misleading here; we should use isCODesktop instead.
| @@ -0,0 +1,72 @@ | |||
| m4_dnl -*- Mode: HTML -*-x | |||
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.
Any good reason to convert to m4?
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.
I converted it to m4 because Coda loads HTML from local file paths and has no HTTP backend. m4 expands the exact relative paths for all js/css files so they resolve correctly. same happens in cool.html.m4
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 fact is, sooner or later this CODA branch code will be merged into main branch, and I’m afraid it could break the current online.
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.
in case of online the adminIntegratorSettings.html produced from this m4 file should be the exact same as before
| _getLocalSettingsUrl: function (): string { | ||
| const settingsLocation: string = app.LOUtil.getURL( | ||
| '/admin/adminIntegratorSettings.html', | ||
| 'adminIntegratorSettings.html', |
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.
Will it affect online? If so, we should add the isCODesktop condition.
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.
I dont think it will affect online as the path is now changed
| select2.js \ | ||
| sanitize-url.js | ||
|
|
||
| if !ENABLE_MOBILEAPP |
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.
There’s no need to remove l10n.js here for mobile. The problem with the current code is that adminIntegratorSettings is built with the admin-js bundle. This reminds me of my Hackweek patch (#11775). Similarly, I think we should either extract this into a separate bundle or ignore translations for now.
qt/coda-qt.cpp
Outdated
| Poco::JSON::Parser parser; | ||
| auto obj = parser.parse(payload).extract<Poco::JSON::Object::Ptr>(); | ||
|
|
||
| const std::string fileName = obj->getValue<std::string>("fileName"); | ||
| const std::string filePath = obj->getValue<std::string>("filePath"); | ||
| const std::string content = obj->getValue<std::string>("content"); |
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.
I think this kind of common, cross-platform code can be moved to /common, similar to how we did it for RecentFiles (https://github.com/CollaboraOnline/online/blob/distro/collabora/coda-25.04/common/RecentFiles.cpp). This would make it much easier to implement and reuse across platforms.
qt/coda-qt.cpp
Outdated
| Poco::Path(base).append("settings/userconfig/browsersetting/browsersetting.json"); | ||
|
|
||
| const Poco::Path viewSettingPath = | ||
| Poco::Path(base).append("settings/userconfig/viewsetting/viewsetting.json"); |
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.
I would not prefer hardcoded path for every setting files.
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 path is already hardcoded in the SettingIframe.PATH object in AdminIntegratorSettings.ts
Signed-off-by: Shardul Vikram Singh <[email protected]> Change-Id: I390c6a1e17f9c2414e885c36e1e042157217aaec
b570299 to
ba694e2
Compare
The dark mode and compact mode options in browsersetting now get applied immediately |
Summary
TODO
Checklist
make prettier-writeand formatted the code.make checkmake runand manually verified that everything looks okay