Skip to content

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.

Sign up for GitHub

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

Merged
katysaintin merged 2 commits into from
Jan 31, 2025
+208 −33
Merged

Improve layout management and fix layout issue #3250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
24086c2
Add a feature to configure the default layout, fix layout management and
katysaintin Jan 31, 2025
8057819
Add comments for new preferences see PR review #3250
katysaintin Jan 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 60 additions & 7 deletions core/framework/src/main/java/org/phoebus/framework/workbench/Locations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,20 @@ public static void initialize()

private static void initInstall() throws Exception
{
//First force logging level to CONFIG and memorize user level
Level userLoggingLevel = logger.getLevel();
logger.setLevel(Level.CONFIG);

// Check for location of installation,
// i.e. the directory that should contain the lib/
// and doc/ folders.
String phoebus_install = System.getProperty(PHOEBUS_INSTALL);
String foundFrom = "$("+ PHOEBUS_INSTALL +") system property";
if (phoebus_install == null)
{
//If the environment variable is not set
//Read from the workbench preferences before
foundFrom = "from the framework*.jar archive";
// Determine location of this class
// During development in the IDE, it's /some/path/phoebus/core/framework/target/classes
// In the product, it's /some/path/lib/framework*.jar
Expand All @@ -63,21 +71,58 @@ private static void initInstall() throws Exception
phoebus_install = path.getAbsolutePath();
System.setProperty(PHOEBUS_INSTALL, phoebus_install);
}
if(phoebus_install != null) {
logger.log(Level.CONFIG, "phoebus_install is set to {0} found from {1}", new Object[] {phoebus_install , foundFrom});
}
//Put back user logging level
logger.setLevel(userLoggingLevel);
}

private static void initUser()
{
String phoebus_user = System.getProperty(PHOEBUS_USER);
//First force logging level to CONFIG and memorize user level
Level userLoggingLevel = logger.getLevel();
logger.setLevel(Level.CONFIG);

String folder_name_preference = System.getProperty(FOLDER_NAME_PREFERENCE);
if (phoebus_user == null)
String foundFrom = "$("+ FOLDER_NAME_PREFERENCE +") system property";
if (folder_name_preference == null)
{
if (folder_name_preference == null)
{
folder_name_preference = ".phoebus";
//Test preference folder_name_preference before
folder_name_preference = WorkbenchPreferences.phoebus_folder_name;
foundFrom = "org.phoebus.framework.workbench/phoebus_folder_name preference in settings.ini file";
if(folder_name_preference == null || folder_name_preference.contains("$(")) {//If it is still null
foundFrom = " default value";
folder_name_preference = ".phoebus";
}
phoebus_user = new File(System.getProperty("user.home"), folder_name_preference).getAbsolutePath();
System.setProperty(PHOEBUS_USER, phoebus_user);
}

logger.log(Level.CONFIG, "folder_name_preference is set to {0} found from {1}", new Object[] {folder_name_preference , foundFrom});

String userHome = System.getProperty(PHOEBUS_USER);
foundFrom = "$("+ PHOEBUS_USER +") system property";
if (userHome == null)
{
//Test preference phoebus_user before
File userFile = WorkbenchPreferences.phoebus_user;
if(userFile != null && userFile.exists()) {
foundFrom = "org.phoebus.framework.workbench/phoebus_user preference in settings.ini file";
userHome = userFile.getAbsolutePath();
}
else {
foundFrom = "$(user.home) system property";
userHome = System.getProperty("user.home");
}
}

logger.log(Level.CONFIG, "user home is set to {0} found from {1}", new Object[] {userHome , foundFrom});

String phoebus_user = new File(userHome, folder_name_preference).getAbsolutePath();
logger.log(Level.CONFIG, "phoebus_user folder is set to " + phoebus_user);
System.setProperty(PHOEBUS_USER, phoebus_user);

//Put back user logging level
logger.setLevel(userLoggingLevel);
}

/** 'Install' location contains the lib/ and doc/ directories.
Expand All @@ -89,6 +134,10 @@ private static void initUser()
*/
public static File install()
{
String install = System.getProperty(PHOEBUS_INSTALL);
if(install == null) {
initialize();
}
return new File(System.getProperty(PHOEBUS_INSTALL));
}

Expand All @@ -102,6 +151,10 @@ public static File install()
*/
public static File user()
{
String user = System.getProperty(PHOEBUS_USER);
if(user == null) {
initialize();
}
return new File(System.getProperty(PHOEBUS_USER));
}
}
6 changes: 6 additions & 0 deletions core/framework/src/main/java/org/phoebus/framework/workbench/WorkbenchPreferences.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ public class WorkbenchPreferences
/** directory of external applications */
@Preference public static File external_apps_directory;

/** Phoebus memento folder name default from $(phoebus.folder.name.preference) System property */
@Preference public static String phoebus_folder_name;

/** Phoebus user home directory contents memento default from $(phoebus.user) System property */
@Preference public static File phoebus_user;

/** external applications */
public static final Collection<String> external_apps;

Expand Down
12 changes: 10 additions & 2 deletions core/framework/src/main/resources/workbench_preferences.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,13 @@
# external_app_alog=Alignment Log,alog,/path/to/alog_viewer

# Directory where external applications are started
# May use system properties
external_apps_directory=$(user.home)
# May use system properties $(user.home)
external_apps_directory=$(user.home)

# Phoebus folder name by default .phoebus
# May use system properties $(phoebus.folder.name.preference)
phoebus_folder_name=$(phoebus.folder.name.preference)
Copy link
Collaborator

@kasemir kasemir Jan 31, 2025

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

Copy link
Contributor Author

@katysaintin katysaintin Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of course, comments added .


# Phoebus user home directory path name by default $HOME or %USERPROFILE%
# May use system properties $(phoebus.folder.name.preference
phoebus_user=$(phoebus.user)
89 changes: 77 additions & 12 deletions core/launcher/src/main/java/org/phoebus/product/Launcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import org.phoebus.ui.application.PhoebusApplication;

import java.io.File;
import java.io.InputStream;
import java.io.FileInputStream;
import java.io.FileOutputStream;
import java.lang.reflect.Method;
Expand All @@ -26,10 +27,55 @@

@SuppressWarnings("nls")
public class Launcher {
private static final String SETTINGS_OPTION = "-settings";
private static final String LOGGING_OPTION = "-logging";
private static final String DEFAULT_LOGGING_FILE="/logging.properties";
private static final String LOGGING_PROP = "java.util.logging.config.file";

public static void main(final String[] original_args) throws Exception {
LogManager.getLogManager().readConfiguration(Launcher.class.getResourceAsStream("/logging.properties"));
final Logger logger = Logger.getLogger(Launcher.class.getName());

// First Handle arguments, potentially not even starting the UI
// settings and logging will define the phoebus.install value if not exist
final List<String> args = new ArrayList<>(List.of(original_args));

//Handle logging first
int indexOf = args.indexOf(LOGGING_OPTION);
String loggingFilePath = null;
InputStream loggingStream = null;
String errorLoggingOption = null;
if(indexOf > 0 && indexOf <= args.size()) {
loggingFilePath = args.get(indexOf + 1);
File loggingFile = new File(loggingFilePath);
if(loggingFile.exists()) {
try {
loggingStream = new FileInputStream(loggingFile);
System.setProperty(LOGGING_PROP, loggingFilePath);
}
catch (Exception e) {
loggingStream = null;
//Memorize errorMessage to log the error after logging instanciation
errorLoggingOption = "Cannot read user logging file " + loggingFile + " " + e.getMessage();
}
}
else {
errorLoggingOption = "User logging file " + loggingFilePath + " not found";
}
}

//If no logging found use the default one
if(loggingStream == null) {
loggingFilePath = Launcher.class.getResource(DEFAULT_LOGGING_FILE).getFile();
loggingStream = Launcher.class.getResourceAsStream(DEFAULT_LOGGING_FILE);
}

//Load logging configuration
LogManager.getLogManager().readConfiguration(loggingStream);
final Logger logger = Logger.getLogger(Launcher.class.getPackageName());
logger.log(Level.CONFIG, "Loading logging configuration from " + loggingFilePath);

if(errorLoggingOption != null) {
logger.log(Level.WARNING, errorLoggingOption);
}

boolean showLaunchError = false;

// Can't change default charset, but warn if it's not UTF-8.
Expand All @@ -46,17 +92,36 @@ public static void main(final String[] original_args) throws Exception {
logger.severe("Default charset is " + cs.displayName() + " instead of UTF-8.");
logger.severe("Add -D\"file.encoding=UTF-8\" to java command line or JAVA_TOOL_OPTIONS");
}
Locations.initialize();
// Check for site-specific settings.ini bundled into distribution
// before potentially adding command-line settings.
final File site_settings = new File(Locations.install(), "settings.ini");
if (site_settings.canRead()) {
logger.log(Level.CONFIG, "Loading settings from " + site_settings);
PropertyPreferenceLoader.load(new FileInputStream(site_settings));

//Handle user settings.ini in order to get Locations informations
//as user home directory and phoebus folder name
//Install path will be set on call of install()
//Locations.initialize();
indexOf = args.indexOf(SETTINGS_OPTION);
File site_settings = null;
if(indexOf > 0 && indexOf <= args.size()) {
String settingsFilePath = args.get(indexOf + 1);
site_settings = new File(settingsFilePath);
}


if(site_settings == null || !site_settings.exists()) {
// Check for site-specific settings.ini bundled into distribution
// before potentially adding command-line settings.
String settingsError = site_settings != null ? site_settings.getAbsolutePath() + " not found" : "is not defined";
logger.log(Level.WARNING, "Settings file " + settingsError);
site_settings = new File(Locations.install(), "settings.ini");
}

if(site_settings != null && site_settings.exists()) {
logger.info("Loading settings from " + site_settings.getAbsolutePath());
FileInputStream fileInputStream = new FileInputStream(site_settings);
if (site_settings.getName().endsWith(".xml"))
Preferences.importPreferences(fileInputStream);
else
PropertyPreferenceLoader.load(fileInputStream);
}

// Handle arguments, potentially not even starting the UI
final List<String> args = new ArrayList<>(List.of(original_args));
final Iterator<String> iter = args.iterator();
int port = -1;
try {
Expand Down
2 changes: 2 additions & 0 deletions core/ui/src/main/java/org/phoebus/ui/Preferences.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ public class Preferences
@Preference public static String default_save_path;
/** layout_dir */
@Preference public static String layout_dir;
/** layout_default */
@Preference public static String layout_default;
/** print_landscape */
@Preference public static boolean print_landscape;
/** ok_severity_text_color */
Expand Down
43 changes: 32 additions & 11 deletions core/ui/src/main/java/org/phoebus/ui/application/PhoebusApplication.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,8 +325,7 @@ private void possiblySelectIniFile(CopyOnWriteArrayList<String> application_para
String errorMessage = errorTitleAndErrorMessage.getValue();

logger.log(Level.SEVERE, errorMessage);

Dialog errorDialog = new Alert(AlertType.ERROR);
Alert errorDialog = new Alert(AlertType.ERROR);
errorDialog.setTitle(errorTitle);
errorDialog.setHeaderText(errorTitle);
errorDialog.setContentText(errorMessage + "\n\n" + Messages.PhoebusWillQuit);
Expand All @@ -348,7 +347,7 @@ private void possiblySelectIniFile(CopyOnWriteArrayList<String> application_para
ObservableList<File> iniFilesInDirectory_ObservableList = FXCollections.observableArrayList(iniFilesInDirectory_List);

if (iniFilesInDirectory_List.size() > 0) {
Dialog<File> iniFileSelectionDialog = new Dialog();
Dialog<File> iniFileSelectionDialog = new Dialog<>();
iniFileSelectionDialog.setTitle(Messages.SelectPhoebusConfiguration);
iniFileSelectionDialog.setHeaderText(Messages.SelectPhoebusConfiguration);
iniFileSelectionDialog.setGraphic(null);
Expand All @@ -357,7 +356,7 @@ private void possiblySelectIniFile(CopyOnWriteArrayList<String> application_para
iniFileSelectionDialog.setHeight(400);
iniFileSelectionDialog.setResizable(false);

ListView listView = new ListView(iniFilesInDirectory_ObservableList);
ListView<File> listView = new ListView<>(iniFilesInDirectory_ObservableList);
listView.getSelectionModel().select(0);

Runnable setReturnValueAndCloseDialog = () -> {
Expand Down Expand Up @@ -419,20 +418,20 @@ private void possiblySelectIniFile(CopyOnWriteArrayList<String> application_para
PropertyPreferenceLoader.load(selectedFile_FileInputStream);
}
} catch (Exception exception) {
displayErrorMessageAndQuit.accept(new Pair(Messages.ErrorLoadingPhoebusConfiguration, Messages.ErrorLoadingPhoebusConfiguration + " '" + selectedFile.getAbsolutePath() + "': " + exception.getMessage()));
displayErrorMessageAndQuit.accept(new Pair<>(Messages.ErrorLoadingPhoebusConfiguration, Messages.ErrorLoadingPhoebusConfiguration + " '" + selectedFile.getAbsolutePath() + "': " + exception.getMessage()));
}
} catch (FileNotFoundException e) {
displayErrorMessageAndQuit.accept(new Pair(Messages.ErrorLoadingPhoebusConfiguration, Messages.ErrorLoadingPhoebusConfiguration + " '" + selectedFile.getAbsolutePath() + "': " + Messages.FileDoesNotExist));
displayErrorMessageAndQuit.accept(new Pair<>(Messages.ErrorLoadingPhoebusConfiguration, Messages.ErrorLoadingPhoebusConfiguration + " '" + selectedFile.getAbsolutePath() + "': " + Messages.FileDoesNotExist));
}
} else {
// Selecting a configuration was cancelled either by pressing the "X"-button or by pressing the ESC-key.
stop();
}
} else {
displayErrorMessageAndQuit.accept(new Pair(Messages.ErrorDuringEvalutationOfTheFlagSelectSettings, Messages.ErrorDuringEvalutationOfTheFlagSelectSettings + ": " + MessageFormat.format(Messages.TheDirectoryDoesNotContainConfigurationFiles, iniFilesLocation_String)));
displayErrorMessageAndQuit.accept(new Pair<>(Messages.ErrorDuringEvalutationOfTheFlagSelectSettings, Messages.ErrorDuringEvalutationOfTheFlagSelectSettings + ": " + MessageFormat.format(Messages.TheDirectoryDoesNotContainConfigurationFiles, iniFilesLocation_String)));
}
} else {
displayErrorMessageAndQuit.accept(new Pair(Messages.ErrorDuringEvalutationOfTheFlagSelectSettings, Messages.ErrorDuringEvalutationOfTheFlagSelectSettings + ": " + MessageFormat.format(Messages.TheArgumentIsNotADirectory, iniFilesLocation_String)));
displayErrorMessageAndQuit.accept(new Pair<>(Messages.ErrorDuringEvalutationOfTheFlagSelectSettings, Messages.ErrorDuringEvalutationOfTheFlagSelectSettings + ": " + MessageFormat.format(Messages.TheArgumentIsNotADirectory, iniFilesLocation_String)));
}
}
}
Expand Down Expand Up @@ -561,7 +560,28 @@ private void startUI(final MementoTree memento, final JobMonitor monitor) throws
Preferences.ui_monitor_period, TimeUnit.MILLISECONDS);

closeAllTabsMenuItem.acceleratorProperty().setValue(closeAllTabsKeyCombination);


//Load a custom layout at start if layout_default is defined in preferences
String layoutFileName = Preferences.layout_default;
if(layoutFileName != null && !layoutFileName.isBlank()) {
layoutFileName = !layoutFileName.endsWith(".memento")? layoutFileName + ".memento" :layoutFileName;
String layout_dir = Preferences.layout_dir;
File parentFolder = null;
File user = Locations.user();
if(layout_dir != null && !layout_dir.isBlank() && !layout_dir.contains("$(")) {
parentFolder = new File(user, layout_dir);
}
if(parentFolder == null) {
parentFolder = user;
}
File layoutFile = new File(parentFolder,layoutFileName );
if(layoutFile.exists()) {
startLayoutReplacement(layoutFile);
}
else {
logger.log(Level.WARNING, "Layout file " + layoutFileName + " is not found");
}
}
}

/**
Expand Down Expand Up @@ -778,8 +798,9 @@ void createLoadLayoutsMenu() {
}

// Get every momento file from the configured layout
if (Preferences.layout_dir != null && !Preferences.layout_dir.isBlank()) {
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);
Copy link
Collaborator

@abrahamwolk abrahamwolk Feb 26, 2025

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?

Copy link
Member

@shroffk shroffk Feb 26, 2025

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.

if (layoutDir.exists()) {
final File[] systemLayoutFiles = layoutDir.listFiles();
if (systemLayoutFiles != null) {
Expand Down
19 changes: 18 additions & 1 deletion core/ui/src/main/java/org/phoebus/ui/application/SaveLayoutHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.phoebus.framework.autocomplete.ProposalService;
import org.phoebus.framework.jobs.JobManager;
import org.phoebus.framework.workbench.Locations;
import org.phoebus.ui.Preferences;
import org.phoebus.ui.autocomplete.AutocompleteMenu;
import org.phoebus.ui.dialog.DialogHelper;
import org.phoebus.ui.docking.DockItem;
Expand Down Expand Up @@ -156,7 +157,23 @@ private static void positionDialog(final Dialog<?> dialog, Stage stage)
private static boolean saveState(List<Stage> stagesToSave, final String layout)
{
final String memento_filename = layout + ".memento";
final File memento_file = new File(Locations.user(), memento_filename);
//Take in account layout dir and add Layout folder in the path
String layout_dir = Preferences.layout_dir;
File user = Locations.user();
File parentFolder = null;
if(layout_dir != null && !layout_dir.isEmpty() && !layout_dir.contains("$(")) {
parentFolder = new File(user , layout_dir);
Copy link
Collaborator

@abrahamwolk abrahamwolk Feb 26, 2025

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.

Copy link
Contributor Author

@katysaintin katysaintin Feb 26, 2025

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

Copy link
Collaborator

@abrahamwolk abrahamwolk Feb 26, 2025

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().)

Copy link
Collaborator

@abrahamwolk abrahamwolk Feb 26, 2025

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.

Copy link
Contributor Author

@katysaintin katysaintin Feb 26, 2025

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.

Hello, sorry I was busy today, I will do the correction tomorrow, (French hour). Katy

Copy link
Contributor Author

@katysaintin katysaintin Feb 27, 2025

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

if(!parentFolder.exists()) {
parentFolder.mkdir();
}
}

if(parentFolder == null) {
parentFolder = user;
}

final File memento_file = new File(parentFolder, memento_filename);

// File.exists() is blocking in nature.
// To combat this the phoebus application maintains a list of *.memento files that are in the default directory.
// Check if the file name is in the list, and confirm a file overwrite with the user.
Expand Down
Loading
Loading