recent projects: cleanup ui #7528

bennetbo · 2024-02-07T22:46:28Z

As the ui for the file finder was recently changed in #7364, I think it makes sense to also update the ui of the recent projects overlay.

Before:

After:

Release Notes:

Improved UI of recent project overlay

bennetbo · 2024-02-07T22:47:57Z

crates/recent_projects/src/recent_projects.rs

+                .child(h_flex().gap_2().child(highlighted_location.names).when(
+                    self.render_paths,
+                    |this| {
+                        this.children(highlighted_location.paths.into_iter().map(|path| {


Can there actually be multiple paths for an entry? I didn't find a case where that is happening. If it can actually happen this might look bad, because the paths are laid out horizontally.

Yea, you can have a Zed workspace that contains multiple folders in it.

Aha thanks, I thought I was missing something there. I never used the "add folder to workspace" feature. I will try to make some improvements then.

Yeah, though the multiple paths only show up if your most recent path (the top one) has multiple paths.

crates/recent_projects/src/recent_projects.rs

bennetbo · 2024-02-07T23:10:49Z

Thanks for the comments, I adjusted the layout to look better for multiple paths

Before:

After:

Should we also trim the path to exclude the folder name because it's repeated just like we do it in the file finder ui?

SomeoneToIgnore

I have a beautiful font (Monocraft) that does horrendous things to Zed's layouts.
So, I quickly checked a few things with this branch and posting it here for visibility.
Original Zed is not much better, but my issue with this PR is that it changes those things so that they are broken different way now, without fixing them

Long file paths are trimmed (see charts-repo-actions-demo entry):

Original Zed is much better here since the path below has enough space:

Multi dir paths force single dir entries to get too tall and all other obvious layouting issues:

Original Zed is much worse but it's not like the new version is qualitatively better:

also, as it's shown on the other original Zed screenshot above, it is able to show multidir project entry normally, if the picker selection changes. So I think original Zed is somewhat closer to being normal than the new version.

mikayla-maki · 2024-02-12T19:08:37Z

but it's not like the new version is qualitatively better

I would disagree, this version at least shows all of the data without being completely broken.

SomeoneToIgnore · 2024-02-12T19:09:54Z

I would disagree: the new version does not show "all of the data", trimming the directories — that is my issue with both versions.

SomeoneToIgnore · 2024-02-12T19:34:32Z

I've checked the new version, and it looks almost done to me: there are still long paths that are trimmed, but the new hover thing makes it much better.

One last thing that seems to be broken is the list scroll, but since the PR is a draft, you might be aware of this 🙂
On the screenshot, I cannot scroll that zed.dev entry further up, even though there are more entries below.

bennetbo · 2024-02-12T20:44:37Z

Thanks @SomeoneToIgnore for testing. Here is a summary of the implementation so far:

Switched back to the vertical layout, because longer paths are easier to display
Paths get now displayed in muted color
There is now a new variant of the Picker which allows items to have different sizes (Picker::list(...))
Added tooltip to show the paths of each project, so when they are cut off you can still see where they point to. Which has the nice side effect that you can actually see the paths of a project when using the project switch button in the upper left corner:

@SomeoneToIgnore yes im aware of the scrolling issue and that's what im currently struggling with.

The list should take all the available space inside the overlay, up to a maximum of what's passed into picker. However if i set the list's size to size_full() it doesn't take any space (just 0). This works fine when using uniform_list.

The only way I can force the list to display is to set a fixed width.
If anyone would be able to help me with this that would be greatly appreciated.
Also thanks to @RemcoSmitsDev for investigating the list implementation with me and trying to solve the layout issue.

Some other ideas which might be nice to have:

Remove duplicated project name from path (zed ~/workspace instead of zed ~/workspace/zed )
Instead of cutting of the characters of the path/project name display some kind of ... (I don't think this is possible in gpui so far though, so probably out of scope)

SomeoneToIgnore · 2024-02-13T08:02:34Z

the list's size to size_full() it doesn't take any space (just 0)

I wonder if it's the same as #6958 (comment) , where we need to wrap a list with canvas and set their size to full.
But alas, I'm not good with gpui layouts so cannot help you here much.

bennetbo · 2024-02-14T19:41:34Z

I wonder if it's the same as #6958 (comment) , where we need to wrap a list with canvas and set their size to full. But alas, I'm not good with gpui layouts so cannot help you here much.

I don't think that will help here, as we're trying to get the list to take as minimal space as possible, up to a maximum height specified by the Picker.
This is essentially what UniformList does, measure the items (actually only the first as there all the same height and multiply that with the amount of visible items) and return the sum of those or the maximum size (bounds) that is available.

However List doesn't do that. The request_layout function does not set any specific size, so we cannot use auto height, as this will always be zero. And we cannot specify a fixed height either, because the picker should shrink down when there is only like one or two items inside the list.

@ConradIrwin helped me to rework the layout behaviour of the list. However it's quite tricky as the items inside the list are loaded lazily and we might not know the actual size of them if they weren't rendered (and measured) before.
As far as I am aware we cannot measure them in layout, because for measuring items we need ElementContext but we only have WindowContext.
So the last commit essentially tries to calculate the size of the items if they were rendered before, however there are various edge cases and im still trying to iron them out.
Im also not sure if this is the actual behaviour we want for List, because the implementation we hacked together can only return its actual size if we painted before and is very specific for my use case.

SomeoneToIgnore · 2024-02-14T20:49:10Z

Oh, sorry for putting you through all this, gpui can hurt sometimes.
I still see something odd with the picker from time to time — if I toggle it, sometimes it hangs in a "collapsed" state for a few seconds, but then gets back to normal if I interact with it or wait a bit:

I think that overall, this is still an improvement, but fear that this bug is more noticeable than before.
On the other hand, I've spent some time nitpicking here already, so not sure how much should I continue with that.
@ConradIrwin do you think we can merge this and fix the issue later? Or maybe you have an idea how to fix that last bit?

bennetbo · 2024-02-14T21:01:49Z

Yes, those are the edge cases that im talking about:

When typing something into the search there seems to be some portion of the list that is still getting rendered (this is the one you screenshotted)
When the list takes less size then the max height the bottom padding seems to be to large (Looks like its doubled)
It feels like its less responsive (this might be because we need to paint once to report the correct size, so we are missing a frame)

I agree with you, that flickering is noticeable and I feel like we do not want to merge this until those pain points are resolved!

bennetbo · 2024-02-14T22:56:14Z

When typing something into the search there seems to be some portion of the list that is still getting rendered (this is the one you screenshotted)

I fixed this, although the visuals are not quite the same as for uniform list. Uniform list will not show up at all, while List will still be rendered (only with the padding) but not show any items

When the list takes less size then the max height the bottom padding seems to be to large (Looks like its doubled)

I took me way too long to figure out that there is no need to include the padding size in the request_layout function (which makes sense)

I believe the visual "bugs" are gone, however the one or two missing frames while typing really bother me (zed is always so fast and responsive, this doesn't fit in). I will look into it tomorrow.

SomeoneToIgnore · 2024-02-15T12:43:51Z

Thank you for keeping squashing them all, that's the hardest part.

I can only confirm the occasional blank panel state (as mentioned above) and an offset issue when scrolling up:
Initial (good):

After pressing up (meh):

bennetbo · 2024-02-16T21:37:49Z

Thanks for the testing once again. This was happening because there was already a max_height on the picker itself, which I used as a workaround for specifying the max_height for the list as well (which is a little too small, because the search box takes some space, so the maximum height is not correct for the list). Therefore the scroll area "thinks" its larger then it actually is -> no scrolling/cut off items.

By reworking the layout implementation I was able to fix the flickering issues, but im afraid I am now stuck on another problem:

The new list implementation fixes the flickering by measuring items in the layout phase (this is cached so there is not much more overhead than before).
I tried to keep the same approach for layout that uniform_list is using. However I don't get the correct results currently...
list seems to not be picking up the max_height of the parent container, while uniform_list is working fine.

Here is the layout "trace" for uniform_list and list, each invocation of the layout callback prints the known_dimensions, available_space, width and height. As you can see the uniform_list callback is called more often then the list one and is missing crucial information about the max_height of the parent. Take a look at the following trace:

// UNIFORM LIST

Layout
[crates/gpui/src/elements/uniform_list.rs:128] known_dimensions = Size { None × None }
[crates/gpui/src/elements/uniform_list.rs:128] available_space = Size { MinContent × MinContent }
[crates/gpui/src/elements/uniform_list.rs:146] width = 220 px
[crates/gpui/src/elements/uniform_list.rs:146] height = 350 px
[crates/gpui/src/elements/uniform_list.rs:128] known_dimensions = Size { None × Some(288 px) }
[crates/gpui/src/elements/uniform_list.rs:128] available_space = Size { MinContent × Definite(272 px) }
[crates/gpui/src/elements/uniform_list.rs:146] width = 220 px
[crates/gpui/src/elements/uniform_list.rs:146] height = 272 px
[crates/gpui/src/elements/uniform_list.rs:128] known_dimensions = Size { Some(542 px) × None }
[crates/gpui/src/elements/uniform_list.rs:128] available_space = Size { Definite(542 px) × MinContent }
[crates/gpui/src/elements/uniform_list.rs:146] width = 542 px
[crates/gpui/src/elements/uniform_list.rs:146] height = 350 px
[crates/gpui/src/elements/uniform_list.rs:128] known_dimensions = Size { None × None }
[crates/gpui/src/elements/uniform_list.rs:128] available_space = Size { Definite(542 px) × MinContent }
[crates/gpui/src/elements/uniform_list.rs:146] width = 542 px
[crates/gpui/src/elements/uniform_list.rs:146] height = 350 px
[crates/gpui/src/elements/uniform_list.rs:128] known_dimensions = Size { None × Some(288 px) }
[crates/gpui/src/elements/uniform_list.rs:128] available_space = Size { Definite(542 px) × Definite(272 px) }
[crates/gpui/src/elements/uniform_list.rs:146] width = 542 px
[crates/gpui/src/elements/uniform_list.rs:146] height = 272 px
Paint
[crates/gpui/src/elements/uniform_list.rs:167] bounds = Bounds {
    origin: Point {
        x: 369 px,
        y: 152 px,
    },
    size: Size { 542 px × 288 px },
}

// LIST

[crates/gpui/src/elements/list.rs:516] known_dimensions = Size { None × None }
[crates/gpui/src/elements/list.rs:516] available_space = Size { MinContent × MinContent }
[crates/gpui/src/elements/list.rs:529] width = 788 px
[crates/gpui/src/elements/list.rs:529] height = 447 px
[crates/gpui/src/elements/list.rs:516] known_dimensions = Size { Some(542 px) × None }
[crates/gpui/src/elements/list.rs:516] available_space = Size { Definite(542 px) × MinContent }
[crates/gpui/src/elements/list.rs:529] width = 542 px
[crates/gpui/src/elements/list.rs:529] height = 447 px
[crates/gpui/src/elements/list.rs:516] known_dimensions = Size { None × Some(463 px) }
[crates/gpui/src/elements/list.rs:516] available_space = Size { Definite(542 px) × Definite(447 px) }
[crates/gpui/src/elements/list.rs:529] width = 542 px
[crates/gpui/src/elements/list.rs:529] height = 447 px
Paint
[crates/gpui/src/elements/list.rs:547] bounds = Bounds {
    origin: Point {
        x: 369 px,
        y: 152 px,
    },
    size: Size { 542 px × 463 px },
}

My current understanding:
When reporting a height that is larger in the max height of the container, the request_measured_layout callback should be called again with known_dimensions.height set to the maximum height of the parent container (288px/272 px without padding in this case).
This is what's happening for uniform_list, but although I pretty much implemented the same for list, taffy never seems to call the request_measured_layout callback with:
[crates/gpui/src/elements/uniform_list.rs:128] known_dimensions = Size { None × Some(288 px) }
[crates/gpui/src/elements/uniform_list.rs:128] available_space = Size { MinContent × Definite(272 px) }
which is the information we need to be able to shrink down the list.

I hope my explanation makes sense, im not sure what's the problem here.
It might just be that im not that familiar with gpui's internals, so it would be awesome if anyone could jump in and help. Also thanks for bearing with me while working on this.

SomeoneToIgnore · 2024-02-16T21:48:05Z

Well this seems to be again very similar to #6958 (comment) comment: at least with my "not scrolling" issue reported there, there was a very similar taffy behavior that did not have any max_height dimensions set.
If that canvas approach does not help, I'm afraid that I'm pretty clueless and can only propose to ask another maintainer to help here, as my gpui layouting skills are also very minimal.

bennetbo · 2024-02-16T22:00:50Z

I managed to finally figure it out! Apparently there is a overflow property on style which I wasn't setting correctly.
I believe it is finally working, I need to do some testing.

bennetbo · 2024-02-16T23:55:31Z

Of course that would have been to easy, my changes seem to break existing layouts using the list element.
Will look into it tomorrow.

…hancement

bennetbo · 2024-02-18T21:51:51Z

Should be ready to review now, you can see a showcase of the current state below:
https://github.com/zed-industries/zed/assets/53836821/c37585eb-9936-4dd6-95b2-5e9e391e9670

Summary:

Handle items with different sizes in the picker correctly
Add an alternative approach for calculating list size based on its children (which we need for auto height)
Show tooltip for paths, so you can still see them when they are cut off
Improved the ui (e.g. paths are now displayed in muted color)
Fix padding displayed for the list elements inside of the picker. Previously, padding was always displayed, even when scrolling down, cutting off text in the process)

A few notes on the implementation:
Im not sure about the design off the list layout code. I introduced an alternative codepath for calculating the size of the list based on its children (ListSizingBehavior::Infer). This was the only solution I was able to come up with to support the new behaviour and not break existing layouts. Would be great to get feedback on this.

SomeoneToIgnore

I believe your approach is what it has to be, after all, so I'm fine with a separate codepath being used.

The recent file picker looks really polished now, thank you for coming up with all those.
Let's fix one big degradation with the rest of the pickers and we'll merge this.

crates/command_palette/src/command_palette.rs

SomeoneToIgnore

Gave it another look and failed to find anything bad, nice! 🎉

Sorry for being a rather good filter but not a good source of help, at least now you seem to be pretty comfortable with gpui as a reward.

bennetbo · 2024-02-19T13:03:54Z

Gave it another look and failed to find anything bad, nice! 🎉

Sorry for being a rather good filter but not a good source of help, at least now you seem to be pretty comfortable with gpui as a reward.

All good, thanks for all the testing you did, that was very helpful!

bennetbo added 2 commits February 7, 2024 23:38

cleanup recent projects ui

72d3f44

update placeholder text for recent projects

79652b3

cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Feb 7, 2024

bennetbo commented Feb 7, 2024

View reviewed changes

crates/recent_projects/src/recent_projects.rs Show resolved Hide resolved

bennetbo marked this pull request as draft February 7, 2024 22:54

fix layout for multiple paths

a9abbfd

bennetbo marked this pull request as ready for review February 8, 2024 00:36

SomeoneToIgnore reviewed Feb 8, 2024

View reviewed changes

ConradIrwin assigned SomeoneToIgnore Feb 9, 2024

SomeoneToIgnore marked this pull request as draft February 10, 2024 00:04

bennetbo added 3 commits February 11, 2024 23:50

support items with different sizes in picker

39aa65b

workaround list auto size for now

ee7df11

add tooltip to recent project matches

dcca326

add list overdraw to fix scrolling

ca5e2e2

wip rework list layout to set actual size

931a1a7

fix clippy

a58db38

handle some edge cases

d0eb157

SomeoneToIgnore mentioned this pull request Feb 16, 2024

Make projects with the same name Distinguishable in "recent projects" selector #7897

Closed

1 task

bennetbo added 2 commits February 16, 2024 22:16

use uniform_list for recent project open_popover

9a2c5db

rework list layout implementation

d1e7e7d

fix scrolling in list by specifying overflow

286fb68

bennetbo added 4 commits February 17, 2024 23:27

Merge remote-tracking branch 'origin/main' into recent-projects-ui-en…

f583648

…hancement

add list layout behavior

f6acd74

use correct scroll offset

89e1248

cleanup and fix scrolling to reveal item issue

edb0c2d

bennetbo marked this pull request as ready for review February 18, 2024 21:54

SomeoneToIgnore reviewed Feb 19, 2024

View reviewed changes

crates/command_palette/src/command_palette.rs Show resolved Hide resolved

bennetbo added 2 commits February 19, 2024 12:06

fix scrolling offset in uniform_list

f50d356

add rustdoc for Picker::list, Picker::uniform_list

44c007b

SomeoneToIgnore approved these changes Feb 19, 2024

View reviewed changes

SomeoneToIgnore merged commit c33efe8 into zed-industries:main Feb 19, 2024
6 checks passed

bennetbo deleted the recent-projects-ui-enhancement branch February 19, 2024 13:04

SomeoneToIgnore mentioned this pull request Feb 25, 2024

projects::OpenRecent list is empty and flashing #8364

Closed

1 task

bennetbo mentioned this pull request Mar 7, 2024

Fix empty tooltip on Recent Projects. #8987

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

recent projects: cleanup ui #7528

recent projects: cleanup ui #7528

bennetbo commented Feb 7, 2024

bennetbo Feb 7, 2024

maxdeviant Feb 7, 2024

bennetbo Feb 7, 2024

mikayla-maki Feb 7, 2024

bennetbo commented Feb 7, 2024 •

edited

Loading

SomeoneToIgnore left a comment •

edited

Loading

mikayla-maki commented Feb 12, 2024

SomeoneToIgnore commented Feb 12, 2024 •

edited

Loading

SomeoneToIgnore commented Feb 12, 2024 •

edited

Loading

bennetbo commented Feb 12, 2024

SomeoneToIgnore commented Feb 13, 2024

bennetbo commented Feb 14, 2024 •

edited

Loading

SomeoneToIgnore commented Feb 14, 2024

bennetbo commented Feb 14, 2024

bennetbo commented Feb 14, 2024 •

edited

Loading

SomeoneToIgnore commented Feb 15, 2024

bennetbo commented Feb 16, 2024 •

edited

Loading

SomeoneToIgnore commented Feb 16, 2024

bennetbo commented Feb 16, 2024

bennetbo commented Feb 16, 2024 •

edited

Loading

bennetbo commented Feb 18, 2024 •

edited

Loading

SomeoneToIgnore left a comment

SomeoneToIgnore left a comment

bennetbo commented Feb 19, 2024

recent projects: cleanup ui #7528

recent projects: cleanup ui #7528

Conversation

bennetbo commented Feb 7, 2024

bennetbo Feb 7, 2024

Choose a reason for hiding this comment

maxdeviant Feb 7, 2024

Choose a reason for hiding this comment

bennetbo Feb 7, 2024

Choose a reason for hiding this comment

mikayla-maki Feb 7, 2024

Choose a reason for hiding this comment

bennetbo commented Feb 7, 2024 • edited Loading

SomeoneToIgnore left a comment • edited Loading

Choose a reason for hiding this comment

mikayla-maki commented Feb 12, 2024

SomeoneToIgnore commented Feb 12, 2024 • edited Loading

SomeoneToIgnore commented Feb 12, 2024 • edited Loading

bennetbo commented Feb 12, 2024

SomeoneToIgnore commented Feb 13, 2024

bennetbo commented Feb 14, 2024 • edited Loading

SomeoneToIgnore commented Feb 14, 2024

bennetbo commented Feb 14, 2024

bennetbo commented Feb 14, 2024 • edited Loading

SomeoneToIgnore commented Feb 15, 2024

bennetbo commented Feb 16, 2024 • edited Loading

SomeoneToIgnore commented Feb 16, 2024

bennetbo commented Feb 16, 2024

bennetbo commented Feb 16, 2024 • edited Loading

bennetbo commented Feb 18, 2024 • edited Loading

SomeoneToIgnore left a comment

Choose a reason for hiding this comment

SomeoneToIgnore left a comment

Choose a reason for hiding this comment

bennetbo commented Feb 19, 2024

bennetbo commented Feb 7, 2024 •

edited

Loading

SomeoneToIgnore left a comment •

edited

Loading

SomeoneToIgnore commented Feb 12, 2024 •

edited

Loading

SomeoneToIgnore commented Feb 12, 2024 •

edited

Loading

bennetbo commented Feb 14, 2024 •

edited

Loading

bennetbo commented Feb 14, 2024 •

edited

Loading

bennetbo commented Feb 16, 2024 •

edited

Loading

bennetbo commented Feb 16, 2024 •

edited

Loading

bennetbo commented Feb 18, 2024 •

edited

Loading