search

BHoM / BHoM_UI

GNU Lesser General Public License v3.0
9 stars 5 forks source link

Ability to save and load ISettings to and from chosen file paths added #412

Closed pawelbaran closed 2 years ago

pawelbaran commented 2 years ago

Issues addressed by this PR

Closes #411

Test files

On SharePoint.

Changelog

Additional comments

The change of signature in SaveSettings does not change the original behaviour without the extra parameter.

pawelbaran commented 2 years ago

@BHoMBot check compliance @BHoMBot check null-handling

bhombot-ci[bot] commented 2 years ago
@pawelbaran to confirm, the following checks are now queued: - `code-compliance` - `documentation-compliance` - `project-compliance` - `branch-compliance` - `dataset-compliance` - `copyright-compliance` - `null-handling` There are 16 requests in the queue ahead of you.
pawelbaran commented 2 years ago

@BHoMBot check compliance @BHoMBot check null-handling

bhombot-ci[bot] commented 2 years ago
@pawelbaran to confirm, the following checks are now queued: - `code-compliance` - `documentation-compliance` - `project-compliance` - `branch-compliance` - `dataset-compliance` - `copyright-compliance` - `null-handling` There are 11 requests in the queue ahead of you.
pawelbaran commented 2 years ago

@BHoMBot check core @BHoMBot check installer @BHoMBot check serialisation @BHoMBot check versioning

bhombot-ci[bot] commented 2 years ago
@pawelbaran to confirm, the following checks are now queued: - `core` - `installer` - `serialisation` - `versioning`
adecler commented 2 years ago

I would like to better understand the user cases first to see if we can create a better solution. There's two things I don't like about this one:

So, ideally, I would prefer a solution that is still not reliant on the user knowing the exact location of the setting folder.

pawelbaran commented 2 years ago

Thanks @adecler for the thoughts. The original source of the proposed change is the need to support multiple Revit-specific settings objects on different Revit versions. So we may have the following cases:

So my idea for this is to use the following file paths: C:\ProgramData\BHoM\Settings\Revit{RevitVersion}{SettingsTypeName}.

On top of the above, we also need to support the ability to load/save the settings on demand, to allow the users to share the settings between each other, not only load them on startup.

The natural solution here is allowing for custom paths on read/write of the settings objects, which is exactly what I have proposed in this PR. With regard to your concerns:

  • Ultimately, we want to be compatible with multiple OS (hence the recent change of .NET framework). This means that we need to keep the dependencies to file system centralised so they can be easily be defined when the BHoM starts in one place and keep the rest of the code OS independent. Anything where the file path is directly used as an input would go against that.

The default paths have priority over the optional ones, so the dependency to the file system stays centralised, with extra ability to use the alternative location when desirable (see my case, or any other platform-specific exception).

  • This expose added complexity to the user. Before, the user just had to know the name of the toolkit they wanted the settings from. Now they need to be aware of internal details like where those settings files are saved.

It is still enough to know the name of the toolkit to load the settings by using Query.Settings, in standard case. However, we also have non-standard cases (Revit is non-standard, yay), and this is what I am trying to unlock.

So to sum up: I am not trying to change the default behaviour or introduce any changes to the general concept, I am just enabling the extra features that may help serve various scenarios.

adecler commented 2 years ago

So my idea for this is to use the following file paths: C:\ProgramData\BHoM\Settings\Revit{RevitVersion}{SettingsTypeName}.

If this is all you need, you can replace the filePath input with a fileName input. That way, the actual file path can still be managed internally. I appreciate it might seems silly right now since we have the filePath hardcoded in the method anyways but it will make more sense when we have a proper centralised BHoMFolder method that return a path based on the OS. That way, even the Grasshopper script that handles the settings could stay OS independent since you only provide the file name.

pawelbaran commented 2 years ago

If this is all you need, you can replace the filePath input with a fileName input. That way, the actual file path can still be managed internally. I appreciate it might seems silly right now since we have the filePath hardcoded in the method anyways but it will make more sense when we have a proper centralised BHoMFolder method that return a path based on the OS. That way, even the Grasshopper script that handles the settings could stay OS independent since you only provide the file name.

This would solve only part of the challenge from what I understand:

With regard to OS independence, I am not sure if I understand what is wrong with keeping the default path as is and adding an option to save/load to and from different location. The standard behaviour would stay OS independent, while the non-standard one (custom path) requires individual care by definition. In this case, I would take the responsibility for breaking the principle of OS independence, knowing that Revit will never go outside Windows OS.

pawelbaran commented 2 years ago

Following the discussion with @adecler, this PR is not needed any more as interaction with settings under custom paths will be realised through ReadFromJsonFile and WriteToJsonFile methods from File_Engine. Therefore closing this one 👍