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

Reorganize buttons in the project manager #50674

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

starry-abyss
Copy link
Contributor

@starry-abyss starry-abyss commented Jul 20, 2021

So each time I recommend Godot to someone, I'm ashamed of the Project Manager (the welcome screen, not the person).

This PR addresses the main problem with it - mixing buttons which affect the project list with buttons which affect the project selected. All other "overhaul" ideas are not going to be included in this PR.

More detailed explanation why this change is suggested at all

Notes:

  1. The loading_label ("Loading, please wait...") is overlapped by Filter projects when invisible, and I don't know how to make Filter projects fixed size regardless of loading_label visibility... Ideally, I wanted this space between Scan and Filter projects given by loading_label, even when invisible.
  2. Technically "Remove Missing" also belongs in the top panel (and "About" does as well), but they don't feel as out of place as the other 3, so not sure

Before:
before

After:
after

Implements #25305
This is a first step towards something like #31342

@starry-abyss starry-abyss marked this pull request as draft July 20, 2021 23:45
@starry-abyss starry-abyss changed the title Start for project manager buttons rearrangement. Project manager buttons rearrangement Jul 20, 2021
@Calinou Calinou added this to the 4.0 milestone Jul 20, 2021
@starry-abyss starry-abyss marked this pull request as ready for review July 21, 2021 00:48
@starry-abyss
Copy link
Contributor Author

Made the buttons a bit larger:

image

@Calinou
Copy link
Member

Calinou commented Jul 22, 2021

I like where this is going so far.

However, to preserve the vertical screen real estate we had previously, can you move the horizontal separator so that it's displayed above the Edit button instead of being displayed below the top bar? Like in this mockup I made:

image

It's not much difference, but considering how small 16:9 displays are still common on laptops, I feel preserving vertical screen real estate is important.

@starry-abyss
Copy link
Contributor Author

@Calinou Done! To be honest, I'm currently working on something else, so unless there is more feedback, it's pretty final. I think one can always do a separate PR later with their refinements.

Copy link
Contributor

@AaronRecord AaronRecord left a comment

Choose a reason for hiding this comment

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

A little knit-picky, but the style guide suggests you end your comments with a period. Other than that the code looks fine to me.

editor/project_manager.cpp Outdated Show resolved Hide resolved
editor/project_manager.cpp Outdated Show resolved Hide resolved
editor/project_manager.cpp Outdated Show resolved Hide resolved
editor/project_manager.cpp Outdated Show resolved Hide resolved
@starry-abyss
Copy link
Contributor Author

@LightningAA Done! I also changed "A bar with buttons and options at top" to "A bar at top with buttons and options".

@akien-mga akien-mga requested a review from Calinou July 23, 2021 14:45
@akien-mga akien-mga modified the milestones: 4.0, 4.x Jan 19, 2023
@Riteo
Copy link
Contributor

Riteo commented Apr 5, 2023

Unless there have been external discussions, this one looks like a nice and generally appreciated PR that got stuck. What's the status of this PR?

@starry-abyss
Copy link
Contributor Author

@Riteo I think I can rebase, but I'd need someone to lobby it for people who make decisions in Godot :'D

@AThousandShips
Copy link
Member

AThousandShips commented Jul 21, 2023

I think I can rebase, but I'd need someone to lobby it for people who make decisions in Godot :'D

@starry-abyss That's not quite how things work here, and without the PR in working conditions you can't really get reviewed, especially as it can only be reviewed based on how it looks and feels, something that requires it to be up to date

There's a very similar PR going currently, #79746 so the two of you should coordinate, but if you can rebase this and bring it up to date, and update the screenshots to match the current setup (with icons etc) this can be looked into

If not then this might be closed in favor of that other PR, as they are very similar and that one is currently active (just opened) but otherwise I'd argue this one has preference as it predates the other one

@starry-abyss
Copy link
Contributor Author

@AThousandShips Thank you for visiting, going to look into it :)

That's not quite how things work here, and without the PR in working conditions you can't really get reviewed

That's a shame, because keeping it updated for 2 years and hoping that somebody considers it would be a waste of time...
As for the other PR, I think one design change per PR is better because with multiple changes there might be disagreement (somebody likes one change, but not the other), and it will take another 2 years then. :D

@YuriSizov
Copy link
Contributor

YuriSizov commented Jul 24, 2023

@starry-abyss I don't think there is a strong understanding that this would improve things. While there is some support, there is always some support for UI changes, so it should be taken with a grain of salt. You are correct that you shouldn't do too much work on something that is stuck in a limbo between some support and lack of approval/consensus.

I think we can agree to reject this for now, given that there hasn't been any resolution in 2 years and UI changes are inherently controversial. That would probably be more fair to you than stringing you along on a basis of hope.

PS. That said, there are some aspirations to improve the project manager overall. So maybe some of your ideas would see the light of day in one form or another.

@AThousandShips
Copy link
Member

AThousandShips commented Jul 24, 2023

Note that I wasn't necessarily saying that you have to keep it up to date constantly, but that a review can't be done properly without the PR being in a working state, the reaction was mostly to that when feedback/attention was given it helps to rebase and bring up to date, the fact that the UI has changed quite a bit in the meantime also means it's hard to review this when you only have the difference with the old layout to go on

Mostly the comment was about that there's no one to lobby to for this, it's a review and decision making progress that is very much attached to the PR itself, not somewhere else to lobby to, that was my main point :)

@starry-abyss
Copy link
Contributor Author

starry-abyss commented Jul 25, 2023

OK, so I rebased, but need some time to upload the update to GitHub...

I think we can agree to reject this for now, given that there hasn't been any resolution in 2 years and UI changes are inherently controversial. That would probably be more fair to you than stringing you along on a basis of hope.

If there is a controversy, could you please bring it to this thread? From what I understand, initially it was decided to postpone it for after 4.0 release because of lack of time. And then it seems to be forgotten?

I think UI changes proposed in various threads here are controversial because they contain multiple changes. My PR contains exactly one small change, and so far nobody gave any objections.

That said, there are some aspirations to improve the project manager overall.

That's what I'm talking about. Everyone wants big overhauls and gets endless debates. The result is project manager UI still looks like it's from previous century. :)

Mostly the comment was about that there's no one to lobby to for this, it's a review and decision making progress that is very much attached to the PR itself, not somewhere else to lobby to, that was my main point :)

Sorry, I meant not the lobby to merge, but lobby to just consider. Somebody to sit in RocketChat and remind about it maybe, IDK how it works. :P

@AThousandShips
Copy link
Member

AThousandShips commented Jul 25, 2023

You can always go over there and link it for attention in those cases, nothing wrong with doing so yourself, won't be taken any less seriously if you advocate for it yourself

@YuriSizov
Copy link
Contributor

If there is a controversy, could you please bring it to this thread?

UI changes are inherently controversial because everyone has different preferences. Even a UI change with an overwhelming support can be canned because it doesn't fit some of the design goals (trust me, I know it first hand). This change with buttons have been discussed multiple times, including in PR review meetings, without much enthusiasm. Like I've said, it's not clear that this is an improvement. You have a different taste and think the current design is somehow outdated, but that's not an agreed upon premise.

That's what I'm talking about. Everyone wants big overhauls and gets endless debates.

Well, my point was that there are project needs that are currently not fulfilled by the project manager. So we are going to address them, which may result in some layout changes and incorporating your suggestions. Or it may be done without any of that. I'm not talking about overhauling for the sake of overhauling/modernizing it. There are specific issues which we need to address, which may lead to some UI changes.

@starry-abyss
Copy link
Contributor Author

Ah, I see. So the key point in this change isn't about taste. It's about the match between what one sees on the screen and the underlying model.

The buttons New Project, Import and Scan are global (higher level), they are for the project manager as a whole. And so are Filter and Sort mechanisms (hence the buttons in question are moved nearby, also because higher level concepts usually go higher in UI, and not because of taste). While actions of most other buttons on the right are dependent on the currently selected project in the list. That's why mixing them is a mess for the user.

Most of the time the user doesn't create a project, instead they do something to the existing one, and it's likely the Edit. In the new layout it's easy to find the Edit button, in the old one it's not. One can of course double-click the project in the list to edit, but you only know about this possibility when you know. :P

This change with buttons have been discussed multiple times, including in PR review meetings, without much enthusiasm. Like I've said, it's not clear that this is an improvement.

I never knew. Nobody told me. Hopefully this comment explains the improvement part!

@YuriSizov
Copy link
Contributor

YuriSizov commented Jul 25, 2023

@starry-abyss That is definitely a better justification than "it still looks like it's from previous century" 🙃 With that argument made we can probably experiment with such a change.

cc @coppolaemilio who's invested in some project manager improvements.

I never knew. Nobody told me.

Sorry about that. There wasn't so much of feedback as there was lack of feedback.

@AThousandShips
Copy link
Member

Seems you accidentally closed the PR by resetting your branch

@starry-abyss starry-abyss reopened this Jul 25, 2023
@starry-abyss
Copy link
Contributor Author

@AThousandShips Yes, this is because I couldn't push my new changes to Github for some reason. In the end had to sync it to stock Godot in the web interface and then re-commit and push.

So now I've updated the code and screenshots for latest Godot.

@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Jul 25, 2023
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of master da81ca6), it works as expected.

Before

Before

After

After

Keyboard shortcuts are still functional.

UI-wise, I think this is an improvement but if you've used the project manager previously, it'll obviously take you some time to get used to the new layout 🙂

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Let's give it a go!

@YuriSizov YuriSizov changed the title Project manager buttons rearrangement Reorganize buttons in the project manager Jul 31, 2023
@YuriSizov YuriSizov merged commit 438d960 into godotengine:master Jul 31, 2023
26 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@KoBeWi
Copy link
Member

KoBeWi commented Jul 31, 2023

Is this separator intended?

@Riteo
Copy link
Contributor

Riteo commented Aug 1, 2023

@KoBeWi yup.

See #50674 (comment)

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.

9 participants