-
Notifications
You must be signed in to change notification settings - Fork 116
Improve layout management and fix layout issue #3250
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
Conversation
configure the user folder with settings.ini
| # May use system properties | ||
| external_apps_directory=$(user.home) | ||
| external_apps_directory=$(user.home) | ||
| phoebus_folder_name=$(phoebus.folder.name.preference) |
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.
Please add some # ...comments... to the preference settings, something that explains these because that all ends up in the online help and https://control-system-studio.readthedocs.io/en/latest/preference_properties.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.
of course, comments added .
|
For what it's worth, you can also implement a default layout by copying some "default_layout.memento" into the "$(HOME)/.phoebus/memento" file. On Linux or Mac, where the launcher script can use /bin/bash, you can implement all sorts of ideas to assemble the |
|
Thank you for the tips. I see also that there is the argument -layout . Thank you. |
| final File layoutDir = new File(Preferences.layout_dir); | ||
| String layout_dir = Preferences.layout_dir; | ||
| if (layout_dir != null && !layout_dir.isBlank() && !layout_dir.contains("$(")) { | ||
| final File layoutDir = new File(Locations.user(), layout_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.
@katysaintin This introduced a change in the semantics of the option org.phoebus.ui/layout_dir: previously, the option org.phoebus.ui/layout_dir specified an absolute path to a directory containing layouts. With this change it always specifies a path relative to Locations.user(). In particular, org.phoebus.ui/layout_dir can no longer be set independently of Locations.user(), which is problematic.
Could you change the option org.phoebus.ui/layout_dir back to be an absolute path?
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 was changed to support defining layout_dir using $(user.home) but we should continue to support absolute paths.
@katysaintin can you update the above so the layout_dir is treated as an absolute path.
We could add support where we also check if the preference includes any variable and resolves the path as relative in those cases.
| File user = Locations.user(); | ||
| File parentFolder = null; | ||
| if(layout_dir != null && !layout_dir.isEmpty() && !layout_dir.contains("$(")) { | ||
| parentFolder = new File(user , layout_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.
Would it be possible to also change (or make backwards-compatible) the save-functionality, to use Locations.user() again?
Before the changes in this pull request, layouts were loaded from both Locations.user() as well as from the directory specified by the option org.phoebus.ui/layout_dir. My understanding is that Locations.user() can be assumed to be a user-specific writable directory, while org.phoebus.ui/layout_dir cannot: one use-case of org.phoebus.ui/layout_dir is for read-only deployments of shared collections of layouts.
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.
Hello, just to be sure, for the modifications :
- layout_dir have to support absolute path
- SaveLayout action should save layout in Location.user() in layout_dir , which in this case a relative path ?
Is that right ? That's means that layout_dir can be absolute or relative ?
I check that ASAP.
Thank you for the review .
Katy
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.
- layout_dir have to support absolute path
Yes, that would restore the old behavior.
- SaveLayout action should save layout in Location.user() in layout_dir , which in this case a relative path ?
I think saving a layout should save it to Location.user(), i.e., a location independent of layout_dir. In particular, saving a layout should not save it to a sub-directory of layout_dir (unless layout_dir happens to be set in such a way as to contain Location.user().)
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.
Is that right ? That's means that layout_dir can be absolute or relative ?
I think it would be best if the implemented functionality is backwards-compatible, so that, by default, layout_dir is an absolute path, and only interpreted as a relative path if the new functionality is enabled.
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.
Is that right ? That's means that layout_dir can be absolute or relative ?
I think it would be best if the implemented functionality is backwards-compatible, so that, by default,
layout_diris an absolute path, and only interpreted as a relative path if the new functionality is enabled.
Hello, sorry I was busy today, I will do the correction tomorrow, (French hour). Katy
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.
Hello, see modifications in PR #3300
backward compatibility see ControlSystemStudio#3250
backward compatibility for layout management see PR #3250
|
For the record, this broke the layout handling at our installation. We set |
Please find a PR to improve the Layout Management feature :
See the screenshot here :

In this way, the user home directory can be configurabled also in settings.ini and not depends on the system. It allows a deployement of a Configuration Folder separated from the Phoebus Installation. And allows all the Phoebus Installations to share the same configuration folder.