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

Updates based on requests #4

Merged
merged 1 commit into from
Oct 18, 2023
Merged

Updates based on requests #4

merged 1 commit into from
Oct 18, 2023

Conversation

mcespedes99
Copy link
Owner

Updates based on the comments from this PR: Slicer/SlicerNeuro#1
Main changes:

  • The module did not work on Windows. Paths separator on Windows is '`, therefore this regexp fails (used two different places): dir_re = r'sub-.+/[a-z]+/'. A quick fix is to execute p=p.replace('\', '/') for the input path string, but it would be nicer to use pathlib or path handling functions (using regex for getting the parent folder is very fragile).

Now I'm using pathlib to find the 2 direct parents of the files (which should correspond to 'datatype' and subject directory, which are used to create output bids folder.

  • Hardcoded paths in cfg file. Why not use application settings? Why not use relative path (within module's resources folder, user home folder, ...)? Conversion failed for me due to the hardcoded paths - I could figure out what's wrong but users would not know why things don't work.

Updated the paths so they can be relative or absolute. Using a relative path to Resources as default.

  • Report any exceptions:

Implemented for main functions in the file.

  • Ask permission from the user before installing any package (see example).

Implemented!

  • Check/uncheck all checkable combobox is really strange. You could replace each combobox with a simple "Check/uncheck all" button.

Done, now using a dynamic button.

  • Change the module title to be human-readable (use space instead of CamelCase), consider following naming conventions (e.g., data importer modules start with Import). For example, the Import Gifti module name would be more consistent with the name of similar modules.

Done.

  • Remember the last selected input folder (it was a bit annoying during testing that I had to reselect the input folder each time I restarted the application or reloaded the module)

Done. It's being saved in the config file now. Only applies for the input folder.

  • Use ctkPathLineEdit for output folder selection. It makes it easy to copy-paste paths, takes up less space, supports auto-complete, automatic saveing of last selected folder, etc.

Done. I had to modify the behaviour a bit as before I was updating the subject list after a change on a directory; however, this was making the package go very slow after I transition to ctkPathLineEdit (specifically, if the user is typing in the path, as the module would try to update the subjects after every character input). Therefore, I added a 'Search subjects' button to allow a more smooth interaction.

  • Consider creating closed surface representation for the segmentations to make them visible in 3D.

No change. I am actually already doing this but only for the segmentations marked as visible.

@mcespedes99 mcespedes99 merged commit 9ae3e0b into main Oct 18, 2023
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.

1 participant