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

[TUI] Add Transfer Tab #281

Merged

Conversation

b-peri
Copy link
Collaborator

@b-peri b-peri commented Dec 1, 2023

Requires #276

Description

This PR adds transfer functionality to the TUI's ProjectManager screen. As proposed previously (#265), this new functionality has been implemented using the following widgets:

  • A new, custom DirectoryTree widget which dynamically displays the transfer status of files in the local project folder.
  • A RadioButton widget through which the user can select their desired transfer mode (from "All", "Top Level", and "Custom")
  • A "Parameters" Container which dynamically renders mode-specific input widgets according to the transfer mode selected above.
  • A Switch widget that allows users to select whether they wish to upload or download their project data.
  • A transfer Button which passes the provided inputs to the appropriate transfer function.
  • A new ConfirmScreen modal dialog, which is displayed after the transfer button is pressed.
  • An options Button, which currently does nothing.

References

Closes #265

Like previous TUI PRs, this PR has only been tested manually. There is also not yet any TUI documentation.

@b-peri b-peri linked an issue Dec 1, 2023 that may be closed by this pull request
@b-peri
Copy link
Collaborator Author

b-peri commented Dec 1, 2023

I've now somewhat arbitrarily set this PR to merge into neuroinformatics-units:tui-dev-branch, but this will likely require some shuffling/rebasing once we start getting through our PR backlog!

@b-peri
Copy link
Collaborator Author

b-peri commented Dec 7, 2023

The core transfer functionality has now been implemented. However, there are a few features that I've decided to leave be for now, that we will likely want to come back to in a later PR:

  1. Most importantly, an Options screen, from which the user can toggle more advanced transfer settings.
  2. Dynamic styling of TransferStatusTree for "Custom" transfers.
  3. Dynamic rendering (and styling) of files contained only on central

@b-peri b-peri marked this pull request as ready for review December 7, 2023 20:05
@b-peri b-peri requested a review from JoeZiminski December 7, 2023 20:06
@JoeZiminski
Copy link
Member

Hey @b-peri just had a quick play around with this, it's frikkin awesome!! Really like the design it is very intutitive, played around with uploading a few files and it worked perfectly, really nice!

I agree with the things to save for later PRs, (1) I think will become part of a more general refactoring and thinking on how to split the transfer options from the general configs. It definately makes sense to split them for the TUI, and maybe even for the API. (2) and (3) are really cool feature but could be aims to add after initial relaese, we can make some issues for these.

Just dumping some misc. thoughts here I made as I used it just now so I don't forget, will come back with proper review ASAP!

  • Will from textualise suggested having the OS send a signal when a filesystem event occurs with watchdog. This would be very useful to refresh the DirectoryTree when a new file is created. They are also working on a PR to keep the DirectoryTree state once it is refreshed which will be very useful, currently it's parked but we can keep an eye on this. In the first instance accepting a trigger from watchdog and updating the DirectoryTree (also when switching tabs, clicking buttons) will be great I'll look into this.
  • could make legend click (show) click (hide) so new user can always have it up as to not forget the color coding
  • For previous pages also I find the textual auto-styling on the folder tree quite distracting (e.g. the background highlighting orange / white, underlining, etc.). We could consider turning some of these off if they become distracting from focusing on the transfer status.
  • as you mentioned we could remove 'central only' here until (3) because this will never show as we only consider local atm.

Looking forward to playing with it more!

@b-peri b-peri changed the base branch from tui-dev-branch to add_validation_to_create_folders December 13, 2023 12:45
Copy link
Member

@JoeZiminski JoeZiminski left a comment

Choose a reason for hiding this comment

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

[EDITING] Hey Brandon this is looking great! I think this feels very intuitive to use and supports a lot of functionality. It is really cool to press the button then see the data appear in the new folders! The code is very well written and can follow easily what is going on and the logic flow. Please see some suggestions on the code and some misc items below:

  1. I exposed some of the testing utils, see tests/quick_make_project.py which might come in handy for quickly making a toy project for manually testing. We can move this code out of the PR before merge.

  2. It would be cool to add the double-click functionality from the create tab to the transfer tab e.g. on the 'Custom' select if you double click a subject folder it adds it to the subject input box. It could append it using a comma seperator rather than replace the existing. This isn't high-priority for this PR though (and actually playing around with it the implementation for the create inputs is a little buggy / confusing, so this should be changed first).

  3. For large files or long transfers, the TUI freezes which isn't a bad thing as it stops people trying to transfer again while previously transfer in progress. However might be nice to change the popup message to 'transferring' ( or even even with dynamic update from rclone's show-progress functionality, though I'm not sure how this would work in practice).

For the create tab, I think moving the 'settings' button from the bottom makes sense as it was a weird place for it. I did like the idea of having some separation between the functional 'Make Project' button and auxillary 'Settings' button. In my head I had it as the region 'this is the doing region where I click to create folders'. I'm not sure if this is completely made up notion specific to me😅 or is a useful distinction to have, if so maybe the 'settings' button could be moved just to the right edge of the screen? Also, there is textual rendering issue in which if the screen is very small so the overflow scroll bar appears, if you scroll to the bottom the buttons are not shown fully. We could tackle these formatting issues on another PR.

Thanks again for this! Feel free to call any comment or suggestion out of scope for this PR and we can create an issue and tackle it later. There are a lot of things to consider for the transfer tab and it is quite complex (the implementation is very neat considering the underlying complexity) so makes sense to split across a few PRs.

datashuttle/tui/custom_widgets.py Outdated Show resolved Hide resolved
datashuttle/tui/screens/modal_dialogs.py Show resolved Hide resolved
datashuttle/tui/tabs/create_folders.py Show resolved Hide resolved
datashuttle/tui/tabs/transfer.py Show resolved Hide resolved
[
(folder, folder)
for folder in canonical_folders.get_top_level_folders()
if (self.project.get_local_path() / folder).exists()
Copy link
Member

Choose a reason for hiding this comment

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

Nice, I wonder if it is worth having this as a datashuttle util

datashuttle/tui/tabs/transfer.py Outdated Show resolved Hide resolved
datashuttle/tui/tabs/transfer.py Outdated Show resolved Hide resolved
datashuttle/tui/tabs/transfer.py Outdated Show resolved Hide resolved
datashuttle/tui/tabs/transfer.py Outdated Show resolved Hide resolved
datashuttle/tui/tabs/transfer.py Outdated Show resolved Hide resolved
@b-peri
Copy link
Collaborator Author

b-peri commented Dec 21, 2023

Hey Joe, thanks again for your extensive review! I've now implemented all of the short-term fixes we discussed during our meeting last week, so this PR should be good to go for now! I'll throw up separate issues for the remaining items.

@b-peri
Copy link
Collaborator Author

b-peri commented Dec 25, 2023

Just some small final notes:

  • I've managed to fix the label format "overwriting" issue that we discussed previously (in which "unstaged" files were not formatting correctly if they had already been styled by TransferStatusTree.format_transfer_label()), so no need to worry about asking the Textual devs about this anymore!
  • At the current moment it seems like datashuttle.utils.rclone.get_local_and_central_file_differences only returns diffs relative to the project's current top level folder. This means that it is not currently possible to display color-coded labels for files under rawdata and derivatives simultaneously. I'm assuming users will be working in rawdata for the most part, so maybe this isn't a huge issue, but I think this would be worthwhile (and probably fairly straightforward) to expand the rclone wrapper function to return all diffs (across both top-level folders) ahead of the initial release of the TUI/DataShuttle!

@JoeZiminski JoeZiminski force-pushed the add_validation_to_create_folders branch from 960a6bb to 79a8917 Compare January 11, 2024 17:20
@JoeZiminski JoeZiminski deleted the branch neuroinformatics-unit:tui-dev-branch January 12, 2024 11:46
@JoeZiminski JoeZiminski reopened this Jan 12, 2024
@JoeZiminski JoeZiminski changed the base branch from add_validation_to_create_folders to tui-dev-branch January 12, 2024 12:42
@JoeZiminski
Copy link
Member

JoeZiminski commented Jan 12, 2024

Hey Brandon,

Thanks a lot for this, that's awesome about the styling! I added a couple of commits, many are prototype / suggestions so let me know what you think:

Try DIrectoryTree with grey guide styling

I tried in this PR to remove all the fancy styling Textualize does to the DirectoryTree guides as I found them distracting from the colouring on the transfer tab. I think it configs tab is also easier to read without all the random flashing colors but it does also look more boring. Also, not all cases are caught because I am still randomly getting orange and green flashes so will require a bit more work if we keep.

``Add extra checkboxes to datatype transfer tab custom` (Note this is further edited in commit
f5bd0ab)

This adds the all, all datatype and all_ses_level_non_datatype checkboxes to the datatype checkboxes on the transfer tab. These work functionallty but the labels are not currently rendered nicely, this can be addressed in #295 (possibly after this PR is merged).

Prototypy commit to extend diff colors to all top ...

This makes a lot of changes to both transfer.py and on the datashuttle-side code, so will need tidying up if we keep it. It addresses your second point and makes both rawdata and derivatives show changes (in "All" mode). In the other two modes only the diffs under the selected top level folder are shown. It also refactors transfer.py so that folders that have 'local_only' within turn green. Folders that have both changed and local_only files within default to orange. This will also work similarly for error (if we ever see such a case).

There is quite a lot of looping here, this is quite fast for smaller projects but if a project have many folders and files it might get quite slow. I'm not certain whether if it is possible to do without so many loops or we can cache some values somewhere. But for now we can keep a note of it and address optimisation later once the need is more pressing and tests are written.

The only other things I can think of (let me know if I missed anything) are:

  1. The folders change their color when they are clicked on, I think due to the focus CSS. I can't figure out a way to override this, I asked on their discord:
Joe — Today at 15:04
Hi everyone, I have a question about styling the DirectoryTree. We are setting custom colors on the text of specific nodes with node.stylize_before (or node.styles.colr = "color" also works). However, when clicking the node the text changes back to it's original color.  I can customise this color by changing TCSS DirectoryTree:focus .tree--cursor  but I would like for the focus to not change the color at all. Is it possible to block this or set the TCSS such that the color doesn't change?

davep — Today at 16:22
To the best of my knowledge (and with a quick glance at the source for Tree), as it stands right now, you can't style the Tree cursor such that there is no styling and the underlying label's style is used.

Joe — Today at 16:49
Thanks a lot for your your help, that makes sense. Do you know if it is possible to subclass the focus-handling machinery for Tree and it stop it triggering a style refresh at all?

davep — Today at 17:33
I believe the code that handles that is within the line drawing code, so not easily overridden, as it stands (I’m not anywhere near a place to check the code right now; but that’s my recollection).

So this seems like it's going to be pain, unfortunately at least I find the color change on click is quite distracting. Maybe we can find a way around this if we need a deeper sublcassing of DirectoryTree when we try to deal with the refreshing behaviour.

  1. I need to align the Create tab inputs to handle multiple inputs the same was as transfer does. I'm still amazed how well this works off the bat for taking multiple lists!

Overall it's really coming along, extremely satisfying to use and looks fantastic!

@JoeZiminski
Copy link
Member

Hey @b-peri sorry for the delay on this! I added a few new features and did some refactorings, please let me know what you think and if you disagree with any changes made.

I think the main thing left to sort out is the refreshing of the tree such that the selected node stays open, for which there is that open PR on textual. I tried to use their fork and the feature worked nicely but was getting random async event errors. We can look into this more but will wait to hear back from Will on that thread before proceeding.

Some changes / new additions:

  • Added a number of new keyboard shortcuts when working with inputs / directorytree (both here and in create folders tab):
    o To fill an input from the DirectoryTree, now howevering over the DirectoryTree with the mouse (when DirectoryTree has focus) and pressing CTRL+F will ‘fill’ the name into the relevant input. Pressing CTRL+A will append the name to the relevant input as you had. This is instead of double clicking which was a bit more convenient but open / close the folder, I dont think there is anything we can do about that. it still feels fairly natural.
    o I tried watchdog and could get it working in a minimum example in a python kernel but could not get it to work in the textual context. I think this might be a problem with threading as watchdog spins up its own thread. This could be a rabbithole so went for a quicker solution, now the DirectoryTree updates when tab is changed and also when CTRL+R is pressed (when DirectoryTree has focus)
    o For Inputs and DirectoryTrees, now pressing CTRL+Q when the widget has focus will copy the input value / tree node path (the path that the mouse is hovered over). Unfortunately CTRL+C is reserved by textual for closing the program, I hope this is not too confusing for people. I tried to override textual’s events and use ESC for exit and CTRL+C for copy, but it still quit on pressing CTRL+C, I wonder if it also relates to the terminal itself. Anyways, did not look further into it for now, it causes problems down the line we can revisit. In a similar context, pressing CTRL+O will open the relevant filepath in the filesystem browser (only tested on windows). If the value of the input is not a path it will handle and show user a modal window.
  • Removed ‘not staged for transfer’ for now as discussed. I also removed ‘Options’ button for now as the configs can be moved here at a later date in the interest of time.
  • Added extra datatype checkboxes for the transfer context, ‘All’, ‘All Datatype’ and ‘All Session Level Non Datatypes’. These names are way too long and confusing, but will address after merge with [TUI] change all_non_ses_level_datatype #295
  • Override textual’s Tree() _render_line function to not apply any CSS when a tree node is clicked to stop the color changing. This is subject to textual issue 4028, need to check in with them because they couldn’t reproduce this problem.
  • Added a ‘Top level folder’ select to the custom transfer settings too. Currently this is not available for Create tab but can add later as described in Figure out top level folder on TUI #298. The actual implementation of this in the transfer-related functions (see the TODO in the docstring) is kind of hacky to ensure the top level folder is reset to what it was before transfer, but potential solutions might also be confusing, will leave for now.

I think once this is merged into tui-dev-branch are basically ready to merge tui-dev-branch! Still some small changes to me made to the TUI but it is basically done!!

@JoeZiminski JoeZiminski merged commit 8bdcfe8 into neuroinformatics-unit:tui-dev-branch Jan 31, 2024
16 checks passed
JoeZiminski added a commit that referenced this pull request Feb 1, 2024
* Refactored input validators into `tui\utils\tui_validators.py`

* Implemented all basic widgets and styling to Transfer tab

* Implemented draft DirectoryTree widget with dynamic styling

* Added FilteredTree widget to omit hidden files

* Implemented core transfer functionality

* Small styling tweaks and refactoring of DirectoryTree tcss

* Refined `DirectoryTree` styling and implemented `ConfirmScreen` modal dialog

* Fixed weird formatting on `CreateFoldersTab` button widget

* Add a make project convenience script to tests, TODO: move this commit.

* Remove validation from transfer tab inputs.

* Use input's value to pass to datashuttle for custom transfer.

* Minor bug fixes & tweaks to styling

* Remove validation code missed on last commit.

* Added double-click functionality and fixed `CreateFoldersTab` settings button styling

* Removed `FilteredTree` and fixed `DirectoryTree` label styling bug

* Added docstrings, minor cleanup, minor bugfixes

* Refactored TransferStatusTree updating functions

* Cleaned up `TransferStatusTree.format_transfer_label`

* Added docstring to `TransferStatusTree.format_transfer_label`

* Refactored all transfer tree updating logic to `TransferStatusTree`

* Corrected minor typo in `TransferTab`

* `TransferTab` > `DirectoryTree` double-click now appends instead of replacing

* Update filetree + diff_paths after switching top level folder

* Try DIrectoryTree with grey guide styling.

* Add extra checkboxes to datatype transfer tab custom.

* Fix change made when playing around.

* Prototypy commit to extend diff colors to all top level folder + propogate new file color up the directories.

* Refactor DatatypeCheckboxes for new transfer tab edition.

* Accept list input to create tab.

* Add custom directory tree to avoid some CSS styling.

* Prototype setting custom datatypes that are not selected to 'Not staged for transfer'

* Revert "Prototype setting custom datatypes that are not selected to 'Not staged for transfer'"

This reverts commit 39fab16.

* Remove 'no staged for transfer' for now.

* Filter out .datashuttle/logs from the directorytrees

* Filter out .datashuttle/logs from the directorytrees

* Add copy key to directorytree.

* Append to input with CTRL+A.

* Refresh directorytree on ctrl+R or tab change - in progress.

* Finalise refreshin on tab change and using CTRL+R

* Finalise refreshin on tab change and using CTRL+R

* Remove options button.

* Add top level folder selector to custom transfer tab.

* Add top level folder selector to custom transfer tab.

* Document and refactoring.

* Finish documentating.

* Small refactor, add CTRL+O to open filesystem browser.

* Add copy and open key press functionality to Inputs.

* Fix bug on transfer files.

* Fix `test_rclone_check` tests after extending to multiple top level folders.

---------

Co-authored-by: JoeZiminski <[email protected]>
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.

[TUI] Add Transfer Tab Functionality to TUI
2 participants