Assetlib: Fix long plugin names breaking the UI #80555
Conversation
AThousandShips
changed the title
|
For screenshots to display you need to put spaces around it, I have fixed it, but keep that in mind for the future |
|
Thanks for the contribution! I don't think horizontal scroll is a good solution for this issue, especially if it changes unpredictability on each page due to the names of assets. I think my suggestion to trim (or alternatively wrap) long names is better, so we keep the column width constant and avoid the bad UX which is having to scroll both horizontally and vertically. |
I thought that might be the case. Thank you for confirming! I apologize for not including this in the PR description. While I was working on this earlier, the first thing I tried was simply truncating the asset title to three characters to see what would happen, and it didn't fix the issue, and there were no other fields that were especially wide, so I started looking at the container setup code for size/margin calculations, and when I saw that horizontal scroll was disabled, I went with the simplest solution to the problem as I saw it. I will need to investigate this further to come up with a proper solution. FWIW, I was not able to find anything in the code that stood out to me as setting a fixed width or that would cause the UI to expand further than its content, so I'm guessing it has something to do with the width of fields other than the asset title, or that there is something going on with the column width calculations, which should be evenly spread between the number of columns determined as I will try to get an update pushed as soon as I can, but I probably won't be able to get anything done until next Saturday since I work six days/week. Thanks! |
|
Upon further investigation, it looks like the nav buttons for the paginated results are what is actually causing the overflow on my system, but I still have not been able to reproduce the uneven column width behavior shown in the screenshots in #80507 on my machine, so now I'm not sure if a fix on my machine would constitute a fix of the issue as reported, and I don't have any other system on which I can test this. I will investigate further and see if I can get to the root of the issue by next weekend. |
|
Sorry for blowing up the conversation. I was able to reproduce the differently sized columns on my local machine by searching for "debug" to find an asset with a sufficiently long name for my DPI settings, so I think just truncating the content length to the calculated column width to keep all columns evenly spaced should fix the issue with a long asset title causing column expansion problems, and I will also look into the scaling/expansion of the nav buttons as well since they cause a similar issue on my machine. I will get to work on these fixes as I am able. I apologize for all of the confusion. I should have put more time up front into reproducing the issue exactly as it was reported instead of focusing on the screen overflow. |
|
Please use rebasing for updating your branch instead of merging, see here |
|
I'm very sorry for the mess that the git history has become. This is my first time contributing to a large project, so I was unprepared to deal with the rebase/merge workflow, and after wrangling with my local fixup commits, I decided to just reset to the state I started in, but this appears to have auto-closed this PR. I will get my fork cleaned up and start working with the correct workflow. I can open a new PR or reopen this one once I have a new commit ready for review. I am still working on a fix for this bug though. |
|
Good luck! If this branch gets cleaned up this can be reopened if you push to it |
|
I was hoping to have something pushed today, but I don't have any more time to work on this right now. I will keep working on this and try to get something pushed next weekend. |
|
@AThousandShips I pushed a commit to my branch that I believe will fix this issue. Can you reopen this PR? Thanks! |
|
It looks like there is actually still an issue with some overflow at 200% scale. I think I can fix this by tweaking the scaling numbers I'm using. I will amend my commit once I have this sorted out. |
|
Got the slight overflow at 200% scale fixed. Should be ready for review now. |
Calinou
added
topic:editor
cherrypick:4.1
|
Out of curiosity, why not use the built-in ellipsis feature found in Label? This was added in 4.0 though, so the fix can't be backported to 3.x if we decide to use that. |
Sorry, I was not aware of this feature, but I can change it if desired. If it is part of On the other hand, if I did it that way, I would personally prefer to just use word wrapping in the What do you think? |
Unfortunately, LinkButton lacks text ellipsis functionality. It's present in Button though, so it could likely be ported in a separate PR. I think we can merge this PR as-is and look at improving the fix later. |
|
Sounds good. Thanks for confirming! |
|
I need to experiment a bit with amending commits. It looks like the soft-reset > force push > commit amended changes > push again process resulted in the PR being auto-closed again. |
|
It auto closes if you make a push that is equivalent to the branch it wants to merge into, as it no longer is a change |
|
Thank you for the detailed review! I will get these changes implemented as soon as I can, but I have a really busy weekend ahead of me, so it may be about a week before I get this resubmitted. I should have this done by Saturday of next week at the latest. |
The UI was extending past the screen width when loading a page diplaying a plugin with an especially long title in the asset store plugin. I implemented a new `EditorAssetLibraryItem::clamp_width` method that checks that the title text is not longer than the column width minus some padding and truncates it if it is. I also noticed that the nav buttons for paginated results were causing the UI to extend past the screen width on higher editor scales since they were hardcoded to show ten page buttons if there were enough results. I modified the pagination slightly to display a dynamic number of nav buttons based on the editor scale in order to fix this other cause of the same problem. I had to use the font of the `title`, which is a `LinkButton` in order to determine the text width, so I added a public getter `get_button_font` to the `LinkButton` class.
|
@YuriSizov I was able to carve out some time for this today. :) I implemented all of the requested changes and pushed the fixed commit. I also added a more detailed commit message this time. This should be ready for review again. Thanks! |
|
I didn't realize I could just re-request a review. Sorry! It's been a long day today. |
|
Thanks! And congrats for your first merged Godot contribution 🎉 |
akien-mga
changed the title
|
Cherry-picked for 4.1.3. |
YuriSizov
removed
the
cherrypick:4.1
Edit:
I think I have this fixed now.
Assets should have their titles truncated with an ellipsis to within the column width now, and I added a tooltip with the full asset title to the ones that get truncated, so that the user can still see the full title without opening the asset page.
I also changed the pagination from having 10 fixed page buttons to using the
EDSCALEto dynamically choose a number of nav buttons to display so that it won't cause the horizontal overflow issue on large scales but will still show more page buttons on smaller scales.It works on my machine at all scales up to 200%. At custom scales above 200% it starts to overflow again, but this happens regardless of the title and nav buttons. I tested this by truncating all titles to three characters and removing the nav buttons completely, and it still overflows at over 200% scale, so I think that is a separate issue and probably affects other parts of the engine UI as well.
I haven't written a unit test for this yet, but I can add one if needed. It will likely be next Saturday before I am able to get to it though.
Here are the updated screenshots:
75% scale:
100% scale:
125% scale:
150% scale:
175% scale:
200% scale:
Original PR:
I removed the line that disables horizontal scrolling in the AssetLib plugin. This fixes the screen overflow issue on my machine, but I am not sure if this is an appropriate solution since there was probably a reason why scroll was disabled in the first place, and I was unable to reproduce the column alignment issues seen in the screenshots posted in #80507 on either the current master or 4.1.1 stable, so I think there may be something platform specific going on here.
My system:
Please advise if enabling horizontal scroll in the AssetLib is a problem, and I will look for another solution.
Additionally, I wanted to write a unit test for this that would basically just check that the editor window size didn't exceed the size of the screen, but I wasn't sure which classes to use for that, and I wanted to get feedback on this solution early, so I'm submitting this now, but if someone can point me in the right direction for where to look for methods related to overall editor window size and screen size, that would be much appreciated!
Here's some screenshots of the AssetLib on my local machine with this fix in place at different editor scales:
100%
125%
150%
175%
200%