Make the File System Dock more user friendly

[Cradmon]

Ok, so I've gone through about half of editor/filesystem_dock.cpp (and subsequently editor/filesystem_dock.h), refactoring and adding usability improvements as I go. Unfortunately, since some of the refactoring and improvements are interlinked; it is difficult to split the commits. However, I will document the changes here:

Summary:

• Allow filesystem dock to maintain collapse state.
• Fix search box bug.
• Limit history remembered.
• Make expand all and collapse all work all the way down the branch, instead of just 2 levels deep.
• Refactoring (Simplifying, renaming, performance, consistency, logical)

editor/filesystem_dock.h:

1. Change button_back to button_tree as it is used to go to the tree view from file view, and shouldn't be confused with going back in history. (logical)
2. Change split_mode to low_height_mode as split_mode is confusing as to which is split mode. (logical)
3. Add history_max_size int to limit the amount of remembered history, so memory isn't unnecessarily used. (memory)
4. Add uncollapsed_paths to create_tree so when creating the res tree, the collapse state may be maintained. (enhancement)
5. Add _update_history to remove duplicated code from _fw_history and _bw_history. (Readability)
6. Add keep_collapse_state to _update_tree so the tree has the option to maintain its collapse state whenever it is updated. (enhancement)
7. Changed _open_pressed to _go_to_file_list, its method implementation has been reduced to create an opposite to _go_to_tree. (Logical)
8. Removed function _go_to_dir as its fuctionality can be achieved using navigate_to_path. (Simplify)

editor/filesystem_dock.cpp:

_create_tree

• new parameter (See editor/filesystem_dock.h pt.4)
• Make sure all tree items along the path are not collapsed, so the selected tree item is always visible. (Enhancement)
• Make sure all previous uncollapsed tree items remain uncollapsed when rebuilding. (Enhancement)

_update_tree

• Allow update tree to remember the path of uncollapsed tree items (before it is cleared) and pass those paths to _create_tree on res tree creation. (Enhancement)
• Change faves to favorite_paths as it is more descriptive. (Readability)
• Move "res://" and get_icon("Folder", "EditorIcons") outside for loop (Performance)
• Change fv to fave as f prefix is used at later stages to refer to file (Consistency)

_notification

NOTIFICATION_RESIZED

• split_mode = low_height_mode (See editor/filesystem_dock.h pt.2)
• Moved the setting of min height to the constructor. (Performance)
• button_back = button_tree (See editor/filesystem_dock.h pt.1)
• Refactored the rest so the non visible part (either tree or file_list) is shown and updated when switching out of low_height_mode. (Performance)
• Removed the call to _fs_changed() as it is a method that should be run when the file system has changed, not when the dock is being resized. (Logical)

NOTIFICATION_ENTER_TREE

• Added String ei = "EditorIcons" and used throughout. (Performance)
• button_back = button_tree (See editor/filesystem_dock.h pt.1)
• _go_to_dir = navigate_to_path. (See editor/filesystem_dock.h pt.8)
• Moved _update_tree into an else block as it shouldn't need to run if the file system is still scanning. (Performance)
• update_tree is using its new parameter (See editor/filesystem_dock.h pt.6)

NOTIFICATION_DRAG_BEGIN

• Changed second if statement to else if, to avoid checking the second if condition if the first is true. (Performance)

NOTIFICATION_EDITOR_SETTINGS_CHANGED

• Set String ei = "EditorIcons" and use throughout. (Performance)
• back_button = button_tree (See editor/filesystem_dock.h pt.1)
• Moved _update_file_display_toggle_button() into the latter if statement, as set_display_mode calls _update_file_display_mode which calls _change_file_display which calls _update_file_display_toggle_button. (Performance)
• update_tree uses new parameter (See editor/filesystem_dock.h pt.6)

_dir_selected

• Changed ti to sel to remain consistent with latter code. (Consistency)
• Set the global path variable to the selected metadata when a directory is selected, as the path should always equal to the selected tree item. (Consistency)
• Set the text for current_path to show the selected item's path. (Useability)
• Push the path change to history. (Consistency)
• low_height_mode = split_mode (See editor/filesystem_dock.h pt.2)
• Replace _open_pressed with _update_files as only _update_files in _open_pressed is needed now the above changes have been made. (Performance)

_favorites_pressed

• Remove the String dir variable, and set the global path variable directly, then use it throughout. (Performance)
• Removed button_favorite->is_pressed() from the if statement, as adding or removing a favorite, shouldn't need to rely on button_favorite being pressed.
• Moved duplicate code outside the if else block. (Readability)

get_selected_path

• No need to add "res://" on to the end of the path as it should already be there in the metadata. (Bug)

navigate_to_path

• Removed the dir_path variable as we can just set the global path variable directly. (Readability)
• Removed _open_pressed and added in the if clause to replicate its functionality. (See editor/filesystem_dock.h pt.7)

_thumbnail_done

• Group if statements together to simplify. (Readability)

_update_files

• Create `String ei = "EditorIcons" and use thoughout. (Performance)
• split_mode = low_height_mode (See editor/filesystem_dock pt.2)
• Refactor the if (use_folders) block by reducing the if statements (Readability)
• Simplify the search box if statement, while fixing the bug when the length == 1 not showing results. (bug)
• Replaced the StringName variables as String should be sufficient, ei was moved to the beginning of the function. (Questionable)
• Created finfo to make the code more descriptive. (Readability)
• Only declare big_icon without initializing it to file_thumbnail yet, as it makes the code more readable if it is initialized in the following if statement. (Readability)
• Move the ..sources.size if statement closer to the end. (Readability)
• Use item_index to make the code more descriptive. (Readability)

_go_to_tree

• Placed code into an if statement, to make it more flexible. (Enhancement)

_go_to_dir

• Removed function as navigate_to_path can do the same thing. (Simplify)

_fs_changed

• Changed set_disabled check to make it easier to understand, from a programming standpoint. (Readability)
• Removed button_favorite->show() as it isn't needed. (Performance)
• _update_tree is using its new parameter (See editor/filesystem_dock.h pt.6)

_set_scanning_mode

• Rearranged code to place it in a more logical order. (Readability)

_fw_history, _bw_history and _update_history

• Refactor duplicated code into _update_history to reduce repetition. (Readability)
• Set the current path text after path change as it needs to update as you move within history. (Consistency)
• _update_tree is using its new parameter (See editor/filesystem_dock.h pt.6)
• Changed set_disabled check to make it easier to understand. (Readability)

_push_to_history

• Move history.resize into the if statement. (Performance)
• On a valid push to history, check history_max_size to keep history size limited. (See editor/filesystem_dock.h pt.3)
• Changed set_disabled check to make it easier to understand. (Readability)

_file_option

• Changed String path to fpath to avoid confusion with the global path variable. (Consistency)

_folder_option

• Removed the child variable as it isn't needed at that scope. (Performance)
• Changed item to selected to be more descriptive. (Readability)
• Combined FOLDER_EXPAND_ALL and FOLDER_COLLAPSE_ALL by setting the variable is_collapsed to identify between the two options. (Simplify)
• Make the collapse options either expand all folders within its hierarchy, or close all of them, instead of just doing that two levels deep. (Enhancement)
• Changed String path to String fpath, to avoid confusion with global path. (Consistency)

_open_pressed

• Changed _open_pressed to be more simple, and changed the name to _go_to_file_list as it is the opposite of _go_to_tree. (Logical)
• split_mode = low_height_mode. (See editor/filesystem_dock.h pt.2)

_search_changed

• Change the function to be more understandable, by checking to see if the file list is visible, before running _update_files. (Readability)

focus_on_filter

• Changed the search box check to a more understandable check. (Readability)

get_drag_data_fw

• Changed String path to fpath to avoid confusion with the global path variable. (Consistency)
• Combine two if statements into one to reduce the amount of code. (Simplify)

can_drop_data_fw

• Changed String path to fpath to avoid confusion with the global path variable. (Consistency)

drop_data_fw

• Changed String path to fpath to avoid confusion with the global path variable. (Consistency)
• _update_tree to use new parameter (See editor/filesystem_dock.h pt.6)

_files_list_rmb_select

• Changed String path to fpath to avoid confusion with the global path variable. (Consistency)
• Reduced the number file_options->add_separator() lines. (Simplify)

select_file

• Replaced functionality with navigate_to_path as it does the same thing. (Simplify)

_file_multi_selected

• Changed String p to fpath to be more descriptive and consistent. (Readability)

_bind_methods

• _open_pressed = _go_to_file_list (See editor/filesystem_dock.h pt.7)
• _go_to_dir = navigate_to_path (See editor/filesystem_dock.h pt.8)

FileSystemDock

• Moved path = "res://" to the top, for more logical ordering.
• Rearranged code to place it in a more logical order. (Logical)
• button_back = button_tree (See editor/filesystem_dock.h pt.1)
• Set the max_history_size, its current size of 20 is a testing value, can be changed. (See editor/filesystem_dock.h pt.3)
• split_mode = low_height_mode (See editor/filesystem_dock.h pt.2)

[HummusSamurai]

Changed _update_tree to remember the collapse state when updating, so the tree doesn't always close.

Thank you! This was always a pain when working on smaller laptop screens.

[HummusSamurai]

Looks good to me, can you rebase the branch so I can test it locally?

[Cradmon]

@HummusSamurai, just rebased the branch, and split the commit.
Feel free to test :)

[akien-mga]

Sorry, haven't had the opportunity to test it yet, but I still intend to. Could you rebase to solve the merge conflict?

[Cradmon]

@akien-mga
That's cool.
I've rebased the branch and updated the pr notes.

Quick question:
Are these detailed pr notes helpful in speeding up the testing process?
Or are they a little too much?
I'd just like to know so I can optimise any future pr requests.

[akien-mga]

Are these detailed pr notes helpful in speeding up the testing process?
Or are they a little too much?

Can't really complain when people give extension details about what they're doing, but it's true that it's a bit verbose and takes a while to review. But more than the verbosity of your explanations, what makes the review difficult is the sheer amount of changes - most are trivial, but given that so many unrelated changes are intertwined we still need to review line by line to spot potential regressions.

For a future PR where you want to give explanations (which is great), feel free to put comments on the relevant lines using the GitHub interface, it would make it easier to read your explanations and the code changes at the same time. No need to detail things such as renaming variables for readability, that's self explanatory (at most we can discuss the name changes if there was a need for a consensus).

[Cradmon]

Thanks for the feedback @akien-mga.

I do definitely need to change my workflow to split the commits into more manageable chunks.
Right now, as I try to learn the Godot code base, my workflow involves selecting a file and experimenting with it to find out how it works, this is what leads to all the intertwined changes. The advantage of this is that I can make larger scale changes with more confidence, and hopefully make better refactoring choices.

Thats a great tip on using the github interface for commenting, I'll start using it 👍.
I guess leaving out self-explanatory changes is a good call, after all anyone with a query on a change will just comment on the PR.

[akien-mga]

From a cursory glance the changes look ok, let's get it tested in production and we'll see how it goes. I'm all for refactoring classes which have grown a bit unwieldy over time, but it's not trivial to assess the changes quickly without having them tested by many users. :)

[Cradmon]

I understand it must have been difficult to review. I thank you for your patience in reviewing it.
It was mainly down to my initial workflow as to why it became complex. Future PRs should hopefully be more reviewer friendly :)