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.

Sign up for GitHub

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

Jump to bottom

Improve file sorting #43018

Merged
merged 1 commit into from
Oct 26, 2020
Merged

Improve file sorting #43018

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions editor/editor_file_system.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,11 @@ bool EditorFileSystemDirectory::get_file_import_is_valid(int p_idx) const {
return files[p_idx]->import_valid;
}

uint64_t EditorFileSystemDirectory::get_file_modified_time(int p_idx) const {
ERR_FAIL_INDEX_V(p_idx, files.size(), 0);
return files[p_idx]->modified_time;
}

String EditorFileSystemDirectory::get_file_script_class_name(int p_idx) const {
return files[p_idx]->script_class_name;
}
Expand Down
1 change: 1 addition & 0 deletions editor/editor_file_system.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ class EditorFileSystemDirectory : public Object {
StringName get_file_type(int p_idx) const;
Vector<String> get_file_deps(int p_idx) const;
bool get_file_import_is_valid(int p_idx) const;
uint64_t get_file_modified_time(int p_idx) const;
String get_file_script_class_name(int p_idx) const; //used for scripts
String get_file_script_class_extends(int p_idx) const; //used for scripts
String get_file_script_class_icon_path(int p_idx) const; //used for scripts
Expand Down
79 changes: 69 additions & 10 deletions editor/filesystem_dock.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,10 @@ bool FileSystemDock::_create_tree(TreeItem *p_parent, EditorFileSystemDirectory
}

// Create items for all subdirectories.
for (int i = 0; i < p_dir->get_subdir_count(); i++) {
bool reversed = file_sort == FILE_SORT_NAME_REVERSE;
for (int i = reversed ? p_dir->get_subdir_count() - 1 : 0;
reversed ? i >= 0 : i < p_dir->get_subdir_count();
reversed ? i-- : i++) {
Comment on lines +91 to +93
Copy link
Member

Choose a reason for hiding this comment

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

IMO that might be more readable as two separate for loops in if and else branches.

Copy link
Member

Choose a reason for hiding this comment

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

Likely also easier to unroll/optimize for compilers.

Copy link
Member

Choose a reason for hiding this comment

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

Well I see you have the same case further down on a bigger for loop, where the duplication might not be wanted. Your call :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this is the best solution I came up with for now.
I could have done something like storing the data in a list, like how it's done for files. But it was over-complicating things for little gain. It though duplicating the whole loop (the big one), removing everything to a function, only to be able to iterate over it in reverse was a little bit too much too.
So I went for this solution and kept things consistent for the too places it was used in. I agree it's not very readable, but I think the "reversed" variable name is at least obvious on what this does.

parent_should_expand = (_create_tree(subdirectory_item, p_dir->get_subdir(i), uncollapsed_paths, p_select_in_favorites, p_unfold_path) || parent_should_expand);
}

Expand Down Expand Up @@ -119,14 +122,13 @@ bool FileSystemDock::_create_tree(TreeItem *p_parent, EditorFileSystemDirectory
fi.name = p_dir->get_file(i);
fi.type = p_dir->get_file_type(i);
fi.import_broken = !p_dir->get_file_import_is_valid(i);
fi.modified_time = p_dir->get_file_modified_time(i);

file_list.push_back(fi);
}

// Sort the file list if needed.
if (file_sort == FILE_SORT_TYPE) {
file_list.sort_custom<FileInfoExtensionComparator>();
}
_sort_file_info_list(file_list);

// Build the tree.
for (List<FileInfo>::Element *E = file_list.front(); E; E = E->next()) {
Expand Down Expand Up @@ -615,6 +617,7 @@ void FileSystemDock::_search(EditorFileSystemDirectory *p_path, List<FileInfo> *
fi.type = p_path->get_file_type(i);
fi.path = p_path->get_file_path(i);
fi.import_broken = !p_path->get_file_import_is_valid(i);
fi.modified_time = p_path->get_file_modified_time(i);

if (_is_file_type_disabled_by_feature_profile(fi.type)) {
// This type is disabled, will not appear here.
Expand All @@ -629,6 +632,54 @@ void FileSystemDock::_search(EditorFileSystemDirectory *p_path, List<FileInfo> *
}
}

struct FileSystemDock::FileInfoTypeComparator {
bool operator()(const FileInfo &p_a, const FileInfo &p_b) const {
// Uses the extension, then the icon name to distinguish file types.
String icon_path_a = "";
String icon_path_b = "";
Ref<Texture2D> icon_a = EditorNode::get_singleton()->get_class_icon(p_a.type);
if (icon_a.is_valid()) {
icon_path_a = icon_a->get_name();
}
Ref<Texture2D> icon_b = EditorNode::get_singleton()->get_class_icon(p_b.type);
if (icon_b.is_valid()) {
icon_path_b = icon_b->get_name();
}
return NaturalNoCaseComparator()(p_a.name.get_extension() + icon_path_a + p_a.name.get_basename(), p_b.name.get_extension() + icon_path_b + p_b.name.get_basename());
}
};

struct FileSystemDock::FileInfoModifiedTimeComparator {
bool operator()(const FileInfo &p_a, const FileInfo &p_b) const {
return p_a.modified_time > p_b.modified_time;
}
};

void FileSystemDock::_sort_file_info_list(List<FileSystemDock::FileInfo> &r_file_list) {
// Sort the file list if needed.
switch (file_sort) {
case FILE_SORT_TYPE:
r_file_list.sort_custom<FileInfoTypeComparator>();
break;
case FILE_SORT_TYPE_REVERSE:
r_file_list.sort_custom<FileInfoTypeComparator>();
r_file_list.invert();
break;
case FILE_SORT_MODIFIED_TIME:
r_file_list.sort_custom<FileInfoModifiedTimeComparator>();
break;
case FILE_SORT_MODIFIED_TIME_REVERSE:
r_file_list.sort_custom<FileInfoModifiedTimeComparator>();
r_file_list.invert();
break;
case FILE_SORT_NAME_REVERSE:
r_file_list.invert();
break;
default: // FILE_SORT_NAME
break;
}
}

void FileSystemDock::_update_file_list(bool p_keep_selection) {
// Register the previously selected items.
Set<String> cselection;
Expand Down Expand Up @@ -718,9 +769,11 @@ void FileSystemDock::_update_file_list(bool p_keep_selection) {
if (efd) {
fi.type = efd->get_file_type(index);
fi.import_broken = !efd->get_file_import_is_valid(index);
fi.modified_time = efd->get_file_modified_time(index);
} else {
fi.type = "";
fi.import_broken = true;
fi.modified_time = 0;
}

if (searched_string.length() == 0 || fi.name.to_lower().find(searched_string) >= 0) {
Expand Down Expand Up @@ -762,7 +815,10 @@ void FileSystemDock::_update_file_list(bool p_keep_selection) {
files->set_item_icon_modulate(files->get_item_count() - 1, folder_color);
}

for (int i = 0; i < efd->get_subdir_count(); i++) {
bool reversed = file_sort == FILE_SORT_NAME_REVERSE;
for (int i = reversed ? efd->get_subdir_count() - 1 : 0;
reversed ? i >= 0 : i < efd->get_subdir_count();
reversed ? i-- : i++) {
String dname = efd->get_subdir(i)->get_name();

files->add_item(dname, folder_icon, true);
Expand All @@ -782,6 +838,7 @@ void FileSystemDock::_update_file_list(bool p_keep_selection) {
fi.path = directory.plus_file(fi.name);
fi.type = efd->get_file_type(i);
fi.import_broken = !efd->get_file_import_is_valid(i);
fi.modified_time = efd->get_file_modified_time(i);

file_list.push_back(fi);
}
Expand All @@ -790,9 +847,7 @@ void FileSystemDock::_update_file_list(bool p_keep_selection) {
}

// Sort the file list if needed.
if (file_sort == FILE_SORT_TYPE) {
file_list.sort_custom<FileInfoExtensionComparator>();
}
_sort_file_info_list(file_list);

// Fills the ItemList control node from the FileInfos.
String main_scene = ProjectSettings::get_singleton()->get("application/run/main_scene");
Expand Down Expand Up @@ -2570,8 +2625,12 @@ MenuButton *FileSystemDock::_create_file_menu_button() {

PopupMenu *p = button->get_popup();
p->connect("id_pressed", callable_mp(this, &FileSystemDock::_file_sort_popup));
p->add_radio_check_item("Sort by Name", FILE_SORT_NAME);
p->add_radio_check_item("Sort by Type", FILE_SORT_TYPE);
p->add_radio_check_item("Sort by Name (Ascending)", FILE_SORT_NAME);
p->add_radio_check_item("Sort by Name (Descending)", FILE_SORT_NAME_REVERSE);
p->add_radio_check_item("Sort by Type (Ascending)", FILE_SORT_TYPE);
p->add_radio_check_item("Sort by Type (Descending)", FILE_SORT_TYPE_REVERSE);
p->add_radio_check_item("Sort by Last Modified", FILE_SORT_MODIFIED_TIME);
p->add_radio_check_item("Sort by First Modified", FILE_SORT_MODIFIED_TIME_REVERSE);
p->set_item_checked(file_sort, true);
return button;
}
Expand Down
15 changes: 10 additions & 5 deletions editor/filesystem_dock.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,11 @@ class FileSystemDock : public VBoxContainer {

enum FileSortOption {
FILE_SORT_NAME = 0,
FILE_SORT_NAME_REVERSE,
FILE_SORT_TYPE,
FILE_SORT_TYPE_REVERSE,
FILE_SORT_MODIFIED_TIME,
FILE_SORT_MODIFIED_TIME_REVERSE,
FILE_SORT_MAX,
};

Expand Down Expand Up @@ -267,16 +271,17 @@ class FileSystemDock : public VBoxContainer {
StringName type;
Vector<String> sources;
bool import_broken;
uint64_t modified_time;

bool operator<(const FileInfo &fi) const {
return NaturalNoCaseComparator()(name, fi.name);
}
};
struct FileInfoExtensionComparator {
bool operator()(const FileInfo &p_a, const FileInfo &p_b) const {
return NaturalNoCaseComparator()(p_a.name.get_extension() + p_a.name.get_basename(), p_b.name.get_extension() + p_b.name.get_basename());
}
};

struct FileInfoTypeComparator;
struct FileInfoModifiedTimeComparator;

void _sort_file_info_list(List<FileSystemDock::FileInfo> &r_file_list);

void _search(EditorFileSystemDirectory *p_path, List<FileInfo> *matches, int p_max_items);

Expand Down