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

User Settable Freemocap_data Path #562

Merged
merged 34 commits into from
Apr 29, 2024
Merged

Conversation

philipqueen
Copy link
Collaborator

@philipqueen philipqueen commented Mar 11, 2024

Adds the freemocap_data path to the gui_state so that the user can set their own path location. Uses the current location as the default.

Will also add the blender path to the gui_state, so users with different Blender path locations won't have to reset the path every time.

Todos:

  • Add to gui state
  • Figure out if we want to pass gui state around or use global
    Using a global variable to change fewer files and reduce the amount of passing around of state we do
  • Figure out logging reference
    By moving the gui state check into the get_freemocap_data_folder_path function, the gui state is loaded when configure logging is called, so this happens without moving the logging code (I'm happy to move logging out of the init function in another PR if we want that).
  • Decide where/how to have user set path
    Will make "settings" dialog (dialog is just what Qt calls a subwindow that pops up above the main GUI - like a "Wizard", but wizards are always multiple pages) accessible from the menu bar. This is what I picked up from the video PR review, let me know if that's not accurate. Since we're pushing forward with a new frontend, this will just be settable in the gui_state.json for now.
  • Add Blender to gui state

Steps to Test:

Data folder path:

  1. Switch to this branch, open the gui, and reset the gui state to default (in the menu bar)
    2. Edit the "freemocap_data_folder_path" in freemocap_data/logs_info_and_settings/gui_state.json to some other path on your computer. Choose Set Freemocap Data Folder in the settings tab of the menu bar and change to a different folder.
  2. Do a simple recording/run sample data, and verify you get a full recording folder (logs, recording session) and everything works as expected

Blender Path:

  1. Switch to this branch, open gui, and make sure blender path populates correctly before you do anything else.
  2. Try switching the blender path to a different file (I made a dummy file named the same thing as blender in a different directory)
  3. Close the gui and open, making sure your new blender path is still there.
  4. Reset the gui defaults or set back to your normal blender path

@philipqueen philipqueen marked this pull request as ready for review March 20, 2024 17:58
@aaroncherian
Copy link
Collaborator

Hey, so I was able to get the data path folder changing to work, but it actually took a lot of finagling. I think if we're going to give users this option and have them change it this way, we need to be hyper-specific in our instructions. For example, when changing the path, should you shut FreeMoCap down first, and then change it, after resetting to default? It seemed to me like that's what you need to do?

Also curious - what exactly triggers the creation of the logs_info_and_settings folder? Because I stumbled onto a problem where I switched the path, but then ended up getting this error whenever I tried to download sample data:

`[Errno 2] No such file or directory: 'C:\Users\aaron\Documents\logs_info_and_settings\gui_state.json'

But I'm not 100% sure why I got this error this time, vs. another time when I did get it to work with the same path.

Also, on Windows the path stuff is finnicky. You have to put two \'s for the path (so C:\\Users, not C:\Users). This is esp important because if you just copy over a path, it only copies with one \. It's also very easy to accidentally delete the comma at the end of the freemocap_data_folder_path in the json and not realize it (I only realized because I caught an error about it as it flashed by in the logs).

All of this to say - it worked - but it took genuinely about 30-40 minutes to get it to work properly. If we're putting this option out, I think we need to have a really, ultra-specific instruction list of how to do it, or have a more user-friendly way of handling it.

@philipqueen
Copy link
Collaborator Author

@aaroncherian you're right, I hadn't tested changing it on the fly. When directly editing the file directly, it needs to be while freemocap isn't running. If you do it while it's running, it doesn't know to reload the gui state, and the next time you change something it will just save over your changes. I could reimplement the settings dialog but as a very simple file dialog that let's you pick where you want it.

I believe the log_info_and_settings folder gets created whenever the first thing is saved there - which is whenever you change the gui state, or the first time you close freemocap (all the logs will be saved at that point at the latest).

As far as this error: ``[Errno 2] No such file or directory: 'C:\Users\aaron\Documents\logs_info_and_settings\gui_state.json'` - it logs the error, but in the case it doesn't find the gui state file it just defaults to the standard freemocap data folder, so it shouldn't actually be an issue.

Overall, my takeaway is we need to handle this in the gui, as manually editing the file is probably too fragile, unless we keep it as a "hidden behavior".

@philipqueen
Copy link
Collaborator Author

@aaroncherian check this out for a gui option. If we want it to be settable in the gui, it will require changing some more things. I want to make sure we're happy with how we want to set it in the gui before going too far with it.

I'm trying to make it work with just rebooting the gui when the data folder is changed, but it's not working immediately.

Screen Shot 2024-03-24 at 6 52 10 PM

@aaroncherian
Copy link
Collaborator

@aaroncherian check this out for a gui option. If we want it to be settable in the gui, it will require changing some more things. I want to make sure we're happy with how we want to set it in the gui before going too far with it.

I'm trying to make it work with just rebooting the gui when the data folder is changed, but it's not working immediately.

Screen Shot 2024-03-24 at 6 52 10 PM

Hey yeah, that looks great! I think something simple like that is all we need

@philipqueen
Copy link
Collaborator Author

Ok, I've got this working. I've moved gui_state from the freemocap data folder into the freemocap folder. This means we have a single source of truth for the gui state, and prevented reference errors (having two different freemocap_data folders, with the gui state of each referring to the other folder).

I've tested the functionality with both cloned freemocap and a pip install of this branch, and changing the folder works and persists across opening and closing freemocap.

The only downside is the gui state may not persist across reinstalls or upgrades, although running pip install git+https://github.com/freemocap/freemocap@philip/user_set_data_folder --force-reinstall did preserve gui state.

@philipqueen philipqueen added the ready for review Pull request is ready for final review label Apr 2, 2024
Copy link
Member

@jonmatthis jonmatthis left a comment

Choose a reason for hiding this comment

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

Just do the simple _initUI thing, but otherwise this is probably good!

It freaks me out a little to be changing something a fundamental as the base folder, but it looks like you got it well handled.

Lets merge all the others, and then test this one again (including all functionality from recording/calibration/etc using phyiscal cameras) before merging it to main

@aaroncherian
Copy link
Collaborator

Ran the sample data and did a multi-cam recording and it all worked fine. I noticed though that when you're in the 'Choose FreeMoCap Data Folder' menu, if you exit it or hit the Cancel button, the GUI will go unresponsive for a few moments and then reboot. Is that intentional?

@aaroncherian
Copy link
Collaborator

The Blender stuff also works but:

  1. Can we make this text box read-only? I was just pasting the .exe path in there before realizing that there's a whole dang button for it right below
    image

  2. When I reset to GUI defaults to change back to the original path, it only changed after a restart. This is one of those, 'I'm not sure if the effort to reward ratio is worth it' things, but would it be possible you think to reset the path when the reset default options is clicked?

@philipqueen
Copy link
Collaborator Author

Ok @jonmatthis, this is ready for review again.

I've fixed Aaron's problems above, and we've both tested changing the freemocap data path, and saving the blender path between sessions.

During testing, we found the QLineEdit for the Blender executable path doesn't work. Specifically, if the user types a path, it doesn't get entered as the blender path. I was able to replicate this error on the latest pip release, so I chose to just make this a QLabel to force the user to use the file dialog selection, which works as it is supposed to

Copy link
Member

@jonmatthis jonmatthis left a comment

Choose a reason for hiding this comment

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

Little quibbles

):
if recording_folder_path is None or recording_folder_path == "None":
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to change whatever is calling this so that if the parameter is None then ...set_active_recordin() isn't called at all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's connected directly to a signal in main_window, so we could add a handle_X_signal function in main window for that. As it is now, it seems better to handle the None type once in the function, vs. 8 separate times in the main_window function. Also, as I'll mention in the other conversation, I think setting the active recording to None is the best way of clearing it.

):
if recording_folder_path is None or recording_folder_path == "None":
logger.info("No recording folder path provided - clearing active recording")
Copy link
Member

Choose a reason for hiding this comment

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

...and if we need a way to 'clear' it (do we?), then we can add a new method for ...clear_active_recording() or soemthing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can add a clear_active_recording() method if we would rather be explicit, but just setting the active recording to None gives the behavior we want. This is how .active_recording_info is initialized, and the active recording info widget already displays it well.

@@ -45,17 +46,17 @@ def __init__(self, top_level_folder_path: Union[str, Path], get_active_recording
self._tree_view_widget.setAlternatingRowColors(True)
self._tree_view_widget.setColumnWidth(0, 250)

if self._top_level_folder_path is not None:
self.set_folder_as_root(self._top_level_folder_path)
self._path_label = QLabel(str(self._gui_state.freemocap_data_folder_path))
Copy link
Member

Choose a reason for hiding this comment

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

Is this where you were having issues with Window's paths losing their backspaces?

It may be because you're casting it to a str (where 's aren't rendered bc they are read as 'escape characters' for \n \t etc)

Can we cast it straight to a pathlib.Path() instead of str? Does that work as expected?

One of pathlib's main jobs is to handle windows path backslashes lol 😂

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just for the QLabel, which has to have a str as input. We don't pull data out of the QLabel ever (we don't do np.load(self._path_label.text) for example), so even if it's not represented without backspaces, it won't affect anything

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right, my bad. Makes sense

@philipqueen philipqueen merged commit ff7c013 into main Apr 29, 2024
3 checks passed
@philipqueen philipqueen deleted the philip/user_set_data_folder branch April 29, 2024 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Pull request is ready for final review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants