Skip to content
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

Draft: Revamped Plugin system #21

Open
wants to merge 51 commits into
base: main
Choose a base branch
from
Open

Draft: Revamped Plugin system #21

wants to merge 51 commits into from

Conversation

AKuederle
Copy link
Member

@AKuederle AKuederle commented Apr 27, 2023

This PR implements a new way of how data can be loaded via Plugins.
Instead of creating a loader for a specific file type, this PR allows to create plugins that can generate their own list of data to be loaded.

This is create, if your data is either not linked to a specific file and requires complicated loading logic with multiple files involved, or when you simply want to make it easy for users to just get a specific selection of available datafiles.

@katharina-j-fau and I already used this version to make labling predefined datasets much easier and it also allows for straight forward integration with tpcp-datasets.

The big issue is, that this change to the plugin system is not backwards compatible in the following ways:

  1. All plugins are now class instances and not classes
  2. BaseImporter is renamed to BaseFileImporter
  3. On all plugins the static name is not a get_name method

As I was doing work on the GUI anayway, also intrduced a couple further changes:

  1. (internal) loading of UI files should have less duplicated code
  2. I added Q and W as global shortcuts to shift the viewport left or right by 30% of the current X axis
  3. Fixed some bugs with regards to loading of data

As this is backwards compatible, the biggest issue is, that we need to update all the documentation, which I have not done and don't have a lot of time for at the moment. @katharina-j-fau Would you be interested to help here?

@MalteOlle What do you think of the changes in general? Could you have a look?

Remaining TODOS:

@MalteOlle
Copy link
Collaborator

I took some time to go over the code and did not find anything suspicious. However, I did not really understand what's going on, let's talk "offline".

@MalteOlle
Copy link
Collaborator

I took some time to go over the code and did not find anything suspicious. However, I did not really understand what's going on, let's talk "offline".

Well, looking at the working example it is a bit clearer to me, I just wonder whether we should incorporate the "load from plugin" into the usual loading data dialog. But maybe there are some good reasons not to do it.

This could for example be the main window of the GUI, in case the plugin wants to access something there.
However, this is unlikely to be necessary since plugins receive either the single
:class:`mad_gui.models.GlobalData` or its attribute `PlotData`, which should be sufficient to do everything.
def _configure(self, parent=None, **_) -> Self:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't yet get what this can actually be used for

mad_gui/plugins/base.py Show resolved Hide resolved
@@ -43,6 +43,9 @@ class GlobalData(BaseStateModel):

"""

active_data_loader = Property(None)
data_index = Property(None, dtype=int)
data_label = Property("", dtype=str)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I did not get what this is - add to docstring?

@MalteOlle
Copy link
Collaborator

From the beginning I struggled a bit with now having two buttons for importers. Today we discussed that dynamically changing the already existing LoadDataDialog is not as easy, because it is coded in the .ui files and would require too much work. However, what about we make use of the PluginSelectionDialog when clicking on "Load data" in the app and then depending on whether it is a file loader or a dataset loader we spawn either the LoadDataDialog, which is now the FileLoaderDialog, or if it is dataset loader spawns the FromPluginLoaderDialog. What do you think @AKuederle ?

The reason why I am suggesting this is that then we keep only one "load data" button and thus I hope that it may be less confusing.

Copy link
Collaborator

@MalteOlle MalteOlle left a comment

Choose a reason for hiding this comment

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

two naming suggestions

sampling_rate_hz: float


class BaseDataImporter(BasePlugin):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
class BaseDataImporter(BasePlugin):
class BaseDatasetImporter(BasePlugin):

"""Run this dialog and return the data, that was selected by the user."""
if self.exec_():
return self.final_data_, self.loader_
return None, None


class FromPluginLoaderDialog(QDialog):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
class FromPluginLoaderDialog(QDialog):
class FromDatasetLoaderDialog(QDialog):

or even remove the From. The reason why I would suggest this is, that also the FileLoaderDialog actually also makes use of plugins and therefore, I think FromPluginLoaderDialog is not pinpointed

remove outdated option of pyinstaller
@MalteOlle
Copy link
Collaborator

buildin an executable works locally, however the executable cannot start because of

  File "mad_gui\components\dialogs\plugin_selection\plugin_selection_dialog.py", line 14, in <module>
  File "mad_gui\utils\helper.py", line 90, in load_window_from_file
  File "mad_gui\utils\helper.py", line 60, in resource_path
  File "mad_gui\utils\helper.py", line 76, in _get_resource_path
TypeError: descriptor 'replace' for 'str' objects doesn't apply to a 'WindowsPath' object

I'll try and see if I can fix that within the next days

@MalteOlle
Copy link
Collaborator

buildin an executable works locally, however the executable cannot start because of

  File "mad_gui\components\dialogs\plugin_selection\plugin_selection_dialog.py", line 14, in <module>
  File "mad_gui\utils\helper.py", line 90, in load_window_from_file
  File "mad_gui\utils\helper.py", line 60, in resource_path
  File "mad_gui\utils\helper.py", line 76, in _get_resource_path
TypeError: descriptor 'replace' for 'str' objects doesn't apply to a 'WindowsPath' object

I'll try and see if I can fix that within the next days

Fixed this and also all other builds. Now pipelines for building standalone on windows, macOS, and Ubuntu work again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants