-
Notifications
You must be signed in to change notification settings - Fork 544
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
ENH: Make Slicer application fully portable #5029
Conversation
This would allow using Slicer as a viewer on a DICOM DVD/USB stick, the only thing would need to be fixed for this is to make DICOM database store relative paths for images that are copied into the database (currently all images and thumbnails are stored with absolute paths). |
Unfortunately, there are a number of absolute paths in the .ini files that would cause problems (see below). So, this is not a full solution yet. @jcfr what do you think, should we use <APPLAUNCHER_DIR> placeholder in the .ini files, too? Slicer.ini:
Slicer-NNN.ini:
|
I think the variable name |
I'd very much like to see the application be relocatable. Running Slicer off a CD was the original motivation for having the launcher in the first place (so we could hand out CD at RSNA in like 2002 or something). I agree with @cpinter and prefer Making everything relative to the app path makes sense to me. It'll also help sandbox the app for security. |
What to do with
Should we add any more locations (e.g., specify file in Slicer.ini or Slicer-NNN.ini)? |
I think it would be fine to look in the application directory for a .slicerrc.py and in the user's home directory. I also like the idea of a .slicerrc-NNN.py in case you wanted to do something specific to the version. If we did put it in the .ini file we could also include a application settings panel entry that allows you to browse to set the location and edit it with a simple text editor (and include a link to the readthedocs with information about what you can put there). I still feel like .slicerrc.py should be used with care, since it could be error prone, but we may as well make it fairly easy to use. |
486a76d
to
b703328
Compare
Instead, we should probably use For regular settings only relevant to Slicer (e.g We would also need to provide an interface for reading/writing settings and ensure the placeholder is properly handled. This means directly using |
Using a placeholder would have the advantage that we could automatically detect path variables (now developers need to think about converting to/from relative paths in application settings). However, prohibiting use of QSettings() class would immediately break many things. Instead, we could make a smooth transition: deprecate usage of QSettings and remove its usage from Slicer core now; then gradually remove its usage from extensions (in 1-2 years). |
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 for working on this set of changes.
The implicit meaning of relative path along with the use of the path mapper for the settings panel address all the issues very nicely 👍
Anticipating the change and to simplify this:
should we have method like Last, is there any absolute path save in within CTK widgets ? |
Yes, if we use a placeholder then we could do this automatically. Unfortunately, QSettings
You are right. Unfortunately, there are a few:
|
🙏 That would be neat.
Agreed
It is Description of the feature is here
Agreed. Could be addressed as part of #4722
That would be reasonable. Other approachI am wondering if this type of settings could be updated explicitly on application shutdown or by providing our own sync method that would call https://doc.qt.io/qt-5/qsettings.html#sync. Rewriting of settings value using
|
Unfortunately, value, setValue, and sync methods are not virtual, so we could not reliably override them. I'm now trying to add pathValue() and setPathValue() methods to ctkSettings to simplify the syntax. Instead of:
we could then use:
When CTK classes read/write paths from/to settings, they would try downcasting QSettings to ctkSettings and use pathValue/setPathValue instead of value/setValue. |
Isn’t QSettings something at the heart of a QApplication? I use it more so that other Slicer modules for the custom modules I create. It allows for defining user settings for various widgets at the user location and default settings for all user accounts at the system level. Having multiple users using the same application with the same default settings with the ability for user settings is important for me. I often also store and apply presets for various custom modules in this manner. Not sure what I would do if the plan is to phase out the QSettings() usage in the Slicer application. Being fully portable hasn’t been a concern at all for me. |
Slicer has been always using more than just a simple QSettings() for storing application settings (e.g., it uses a common userSettings and revision-specific revisionUserSettings). Since Python packages must be installed in the Slicer home directory, it cannot be shared between users anymore. Since extensions can depend on various Python packages (with potentially incompatible version requirements), extensions cannot be simply shared between multiple installations of the same Slicer version for the same user. Therefore, we need to make extensions install within the Slicer home directory, too. Since we have everything in one folder, it makes a lot of sense to go one step further and allow the application to be portable. This opens a up a very important use case: bundling a preconfigured Slicer instance to exported data sets as a viewer. It also allows shared installation for many users in a server environment (as extensions can be all copied into the shared Slicer home directory), and also allows easy distribution preconfigured Slicer environments on training courses. QSettings() will not go away completely, it will be just simpler and safer to use app->userSettings() and app->revisionUserSettings(). |
Comments below are for information only and to capture some idea of future discussion
Moving forward, if we only support building Slicer with At application startup, we would check if home folder is writable or not ... and have a relocatable application or not. Function |
It is still useful to allow sharing some user preferences (dark/light mode, DICOM database folder, dialog "don't show again" settings, etc.). With the proposed changes in this pull request, it is possible to choose between a fully isolated application (by having a Slicer.ini file in the Slicer Home folder) or sharing some user preferences between application instances (by not having Slicer.ini in Slicer Home). |
Ok just wanted to throw out some commercial usage of Slicer as this is pretty different from the motivation of making it easy to share the installation with data for sharing in various cases. We still prefer the installation of Slicer/SlicerCustomApp in a system location not a user location. This is because customers have many users/researchers and they setup a local instance and use multiple Windows user accounts for access on the local machine and not a server environment. Installation is restricted to certain machines. Installing of different versions and installation of extensions not included is also restricted. |
You can still keep the current behavior in your custom application by setting Slicer_STORE_SETTINGS_IN_APPLICATION_HOME_DIR to OFF in your custom application. However, Slicer_STORE_SETTINGS_IN_APPLICATION_HOME_DIR actually makes shared installations safer and more predictable, too, as you can preinstall extensions for all users and you can prevent non-admin users from installing extensions. Do you want your users to be able install extensions or Python packages? |
Ok sounds good 👍
This could be beneficial if we want to preinstall some extension that hasn't made it into our SlicerCustomApp yet. We wouldn't support users installing/removing extensions.
At this time, we do not want users to install extensions or python packages. We control the experience in a very specific manner. |
b703328
to
b80624c
Compare
I've moved the path replacement logic to CTK (ctkCoreSettings class) - @jcfr please have a look: commontk/CTK#922 Once this CTK prerequisite is merged, I'll finalize this PR. |
Thanks for working on this 🙏 , I just posted few comments. |
a73ed57
to
c357267
Compare
settings.setValue("Modules/AdditionalPaths", | ||
ctkCoreSettings settings(q->extensionsSettingsFilePath(), QSettings::IniFormat); | ||
settings.setApplicationHomeDirectory(qSlicerCoreApplication::application()->slicerHome()); | ||
settings.setApplicationHomePlaceholder("<APPLAUNCHER_DIR>"); |
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.
Consider using <APPLAUNCHER_SETTINGS_DIR>
, also the ApplicationHomeDirectory
may have to be set to match the location of the settings file instead.
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.
To ensure PythonSlicer
works, could you update to use APPLAUNCHER_SETTINGS_DIR
?
I've tested this on Windows and seems to work well. @jcfr could you test in Linux? Installing a larger extension package that contains loadable and CLI modules (such as SlicerRT or SlicerIGT) and testing if the modules run correctly should be enough. Slicer-(revision).ini and all installed extensions should show up in the (applicationHome)/NA-MIC folder. Maybe you could also test if you put a Slicer.ini file into (applicationHome)/NA-MIC folder then it overrides the global ~/Slicer.ini file. |
To test did you build this branch and create packages of it and also the extensions? |
I've built and packaged extensions myself, but probably extension packages that you download from the dashboard should work, too. |
A package is being built on metroxplex |
Build failed with the following error:
|
ctkCoreSettings.h is the new class in CTK. Probably you just need to do a top-level build to get CTK updated. |
Should the topic be rebased or CTK updated ? It currently checkout this version: Slicer/SuperBuild/External_CTK.cmake Lines 58 to 62 in c357267
Whereas this one should be used: |
You are right, the branch has to be rebased on latest Slicer master branch to get the new CTK hash. |
c357267
to
5fb7f2d
Compare
I've rebased the branch now. It should work now. |
Build in progress, will report back once complete. |
Linux build was successful 👍 TestingPrerequisites:
Summary:
AnalysisAfter installing Specifying
For comparison, after installing
Finally, starting Slicer specifying Proposed solutionsTo address this, the application launcher should be updated so that the path to the user additional settings is read from the main settings instead of being inferred from the QSettings. Optionally, function Error message
|
Thanks a lot for the thorough testing and analysis! We would still want to allow sharing a single Slicer.ini between all Slicer installs (which don't have a local Slicer.ini). So, the Slicer-NNN path cannot be stored in Slicer.ini. Slicer looks for Slicer.ini in <applauncher_dir>/organization and if it is not found there then it looks for it in userprofile/organization folder. Currently, Slicer-NNN.ini is only looked for in <applauncher_dir>/organization or userprofile/organization folder, depending on CMake option chosen during Slicer build. Launcher currently looks for these files only in userprofile/organization. We should resolve these inconsistencies. I think the best behavior would be to look for both ini files in <applauncher_dir>/organization folder first and if it is not found there then in userprofile/organization folder. What do you think? |
Agreed.
To clarify, the idea is to set That way, the launcher will lookup the |
5fb7f2d
to
f998be6
Compare
37a8d31
to
9d1f2c1
Compare
@lassoan I am currently building and testing this PR, Once I am done, I will also integrate the relevant PR in CTK and publish an updated set of launcher binaries. |
I pushed a change in qSlicerExtensionsManagerModelTest (fixes this test, no change in actual application code). |
Slicer install tree was portable but not fully self-contained, as settings and extensions were stored in the user profile folder. Added a new option Slicer_STORE_SETTINGS_IN_APPLICATION_HOME_DIR (enabled by default) that makes Slicer store all settings and extensions in the application home folder, within (organizationName) subfolder (as QSettings default constructor saves settings file into (organizationName) subfolder by default), making the application fully portable. To still allow having settings that are common to all installed application versions for a user (e.g., DICOM database folder, confirmation popup suppressions, ...), the common non-revision-specific is still stored in the user profile directory by default. However, if the user places an (applicationName).ini file in the application home folder/(organization) then that file is used instead. By default, (applicationName)=Slicer and (organization)=NA-MIC (or the organization URL, depending on the operating system), and therefore the .ini files are: (applicationHome)/NA-MIC/Slicer.ini and (applicationHome)/NA-MIC/Slicer-(revision).ini; and extensions are stored in (applicationHome)/NA-MIC/Extensions-(revision). Also updated all path reading/writing in settings files so that paths that are child folders of (applicationHome) are stored as relative paths. All relative paths remain valid when the application is moved to a different location. Fixes Slicer#4966 Fixes Slicer#4722 ---- List of CTKAPPLauncher changes: $ git shortlog v0.1.27..v0.1.28 --no-merges Andras Lasso (1): ENH: Allow user settings file to be in <APPLAUNCHER_DIR> (PR-114) James Butler (1): COMP: Fix deprecation warning C4996 Jean-Christophe Fillion-Robin (3): Begin post-v0.1.27 development [ci skip] Fix deployment updating API key used for release upload CTKAppLauncher v0.1.28 Steve Pieper (2): BUG: fix crash calculating environment keys STYLE: use clearer syntax for constructor ---- List of CTKAppLauncherLib changes: $ git shortlog 692164..a3100a4 --no-merges Andras Lasso (1): ENH: Allow user settings file to be in <APPLAUNCHER_DIR> (PR-114) Jean-Christophe Fillion-Robin (2): Fix deployment updating API key used for release upload CTKAppLauncher v0.1.28
9d1f2c1
to
eaa45ad
Compare
Slicer install tree was portable but not fully self-contained, as settings and extensions were stored in the user profile folder.
Added a new option Slicer_STORE_SETTINGS_IN_HOME_DIR (enabled by default) that makes Slicer store all settings and extensions in the application home folder, within (organizationName) subfolder (as QSettings default constructor saves settings file into (organizationName) subfolder by default), making the application fully portable.
To still allow having settings that are common to all installed application versions for a user (e.g., DICOM database folder, confirmation popup suppressions, ...), the common non-revision-specific is still stored in the user profile directory by default. However, if the user places an (applicationName).ini file in the application home folder/(organizationName) then that file is used instead.
By default, (applicationName)=Slicer and (organizationName)=NA-MIC and therefore the .ini files are: (applicationHome)/NA-MIC/Slicer.ini and (applicationHome)/NA-MIC/Slicer-(revision).ini; and extensions are stored in (applicationHome)/NA-MIC/Extensions-(revision).
re #4966
Prerequisites to merge: