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

Add browse folder and browse file icons #88827

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

adamscott
Copy link
Member

@adamscott adamscott commented Feb 25, 2024

Adds the missing "browse folder" and "browse file" icons and replaces the "Browse" text button of the ProjectDialog, and the FBXImporterManager to an icon button.

New icons: FolderBrowse and FileBrowse

I opened the folder icon in Inkscape (Folder editor/icons/Folder.svg), created circles of 1px of radius, then created an outline of 1px. Subtracted the outline from the folder icon.

Before After
Capture d’écran du 2024-02-25 13-15-58 Capture d’écran du 2024-03-07 16-44-22
Capture d’écran du 2024-02-25 17-50-30 Capture d’écran du 2024-03-07 16-58-15
Capture d’écran du 2024-02-26 08-59-08 Capture d’écran du 2024-03-07 16-56-22
Capture d’écran du 2024-02-28 10-50-13 Capture d’écran du 2024-02-28 10-48-30
Capture d’écran du 2024-02-28 10-50-12 Capture d’écran du 2024-02-28 10-48-26

Relates to #88825

Edits

  • 2024-02-26
    • Added the "browse file" icon
    • Added the browse file to the FBXImporterManager dialog.
    • Added the browse folder to the EditorFileSystemImportFormatSupportQueryBlend dialog.
  • 2024-02-28
    • Changed editor settings icons depending on what is needed
  • 2024-03-07
    • Added back labels to buttons.

@adamscott adamscott requested review from a team as code owners February 25, 2024 18:18
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

My thoughts are the same as #88825 (review)

@MewPurPur
Copy link
Contributor

MewPurPur commented Feb 26, 2024

This one requires a lot more spacing between the folder and the dots, as it's not grid-aligned or multicolored. I think it'll look better if we make the circles a bit bigger too.

@adamscott adamscott changed the title Add browse project icon Add browse folder and browse file icon Feb 26, 2024
@adamscott adamscott changed the title Add browse folder and browse file icon Add browse folder and browse file icons Feb 26, 2024
@adamscott adamscott force-pushed the add-browse-folder-icon branch 2 times, most recently from b702f80 to 27510f5 Compare February 26, 2024 13:30
@adamscott
Copy link
Member Author

I added more space between the dots and the icon (moved from 1px to 1.5px).

And I added the browse file icon! FileBrowse

@adamscott
Copy link
Member Author

I added the browse folder icon to the EditorFileSystemImportFormatSupportQueryBlend dialog. This should wrap up all the "browse" buttons.

Copy link
Contributor

@MewPurPur MewPurPur left a comment

Choose a reason for hiding this comment

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

LGTM (literally)

@AThousandShips
Copy link
Member

Could you try adding it here too?

edit_menu->add_icon_item(get_editor_theme_icon(SNAME("Load")), TTR("Quick Load..."), OBJ_MENU_QUICKLOAD);
edit_menu->set_item_tooltip(-1, TTR("Opens a quick menu to select from a list of allowed Resource files."));
// Add an option to load a resource from a file using the regular file dialog.
edit_menu->add_icon_item(get_editor_theme_icon(SNAME("Load")), TTR("Load..."), OBJ_MENU_LOAD);

@adamscott
Copy link
Member Author

Could you try adding it here too?

edit_menu->add_icon_item(get_editor_theme_icon(SNAME("Load")), TTR("Quick Load..."), OBJ_MENU_QUICKLOAD);
edit_menu->set_item_tooltip(-1, TTR("Opens a quick menu to select from a list of allowed Resource files."));
// Add an option to load a resource from a file using the regular file dialog.
edit_menu->add_icon_item(get_editor_theme_icon(SNAME("Load")), TTR("Load..."), OBJ_MENU_LOAD);

I don't think this PR obsoletes the load logo Load. I think that this PR means to replace the "Browse" text buttons, which don't load/open by themselves files, but only sets the path in the field.

@AThousandShips
Copy link
Member

True, this case here is still more a browse as it assigns a resource from a load, not opens it for editing as such

@adamscott
Copy link
Member Author

@akien-mga Makes me think that I could create a PR to remove the FBX importer dialog as ufbx was merged. The dialog is plain wrong.

@adamscott
Copy link
Member Author

True, this case here is still more a browse as it assigns a resource from a load, not opens it for editing as such

How do we summon this edit menu already?

@AThousandShips
Copy link
Member

It's in the inspector, when you have a resource

@adamscott
Copy link
Member Author

adamscott commented Feb 26, 2024

@AThousandShips But this replaces the current resource, effectively loading it.

image

@AThousandShips
Copy link
Member

AThousandShips commented Feb 26, 2024

It loads it in the sense that it opens it in memory, but the main purpose is to replace the resource there, so I'd say it's the same as browsing for a file path, but it was just an idea :)

It doesn't replace the current resource, just the resource in the property

@Calinou
Copy link
Member

Calinou commented Feb 26, 2024

Should we use this new icon for file/directory properties in EditorInspector (e.g. in the Editor Settings)?

image

@adamscott adamscott force-pushed the add-browse-folder-icon branch 2 times, most recently from 06041ee to cea5b0b Compare February 28, 2024 15:49
@adamscott
Copy link
Member Author

Should we use this new icon for file/directory properties in EditorInspector (e.g. in the Editor Settings)?

image

@Calinou Done! See the updated images on the PR description.

@adamscott adamscott force-pushed the add-browse-folder-icon branch 2 times, most recently from a3948c8 to 42961e5 Compare March 7, 2024 21:59
@adamscott
Copy link
Member Author

@Mickeon @MewPurPur @akien-mga Following this comment, I added back text labels for the "create project" dialog, the Blender dialog and the FBX dialog. See the PR description for the updated images.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me.

modules/gltf/editor/editor_scene_importer_blend.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga modified the milestones: 4.x, 4.3 Mar 8, 2024
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

All is well what ends well

modules/gltf/editor/editor_scene_importer_blend.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga merged commit 48ad83a into godotengine:master Mar 12, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants