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 centered view mode #3239

Closed
wants to merge 2 commits into from
Closed

Conversation

ldubos
Copy link

@ldubos ldubos commented Jul 29, 2022

Resolves #3203

Add a new toggle_centered_view command in Window mode and a centered-views to the editor config, change the behaviour of the recalculate function in Tree to center a View when it has the is_centered set to true to center it in the available space if the View isn't a child of a vertical container with more than 1 children.

I found that is the "standard" behaviour of the "centered mode" in most editors I use.

image

@CptPotato
Copy link
Contributor

Nice!

One question though: how is the view width defined?
Looking at the diff this isn't immediately obvious to me. I personally would want to specify the width either through a setting or at runtime when calling the command.

@ldubos
Copy link
Author

ldubos commented Jul 29, 2022

@CptPotato for the moment we can't define the width unfortunately I don't really know how I can make it configurable and updatable during a session, it seems really complicated, also I don't take the gutters_offset so the width = the total width not the inner width.

and I calculated the centered width like this: centered_width = min(max(available_width / 2, 120), available_width) that means if the available_width / 2 < 120 we set it to 120 and if the result is greater than the available_width we set it to available_width.

So you always have at least 120 columns (with gutters) in centered mode or the available_width / 2 if you have more space available, and the fallback is just the available space so no centering.

@kirawi kirawi added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Sep 13, 2022
@ShalokShalom
Copy link

@ldubos I love this PR, and see that it is currently prevented from being merged by conflicts.

Do you see any chance to resolve them?

@@ -57,6 +57,7 @@ on unix operating systems.
| `rulers` | List of column positions at which to display the rulers. Can be overridden by language specific `rulers` in `languages.toml` file. | `[]` |
| `bufferline` | Renders a line at the top of the editor displaying open buffers. Can be `always`, `never` or `multiple` (only shown if more than one buffer is in use) | `never` |
| `color-modes` | Whether to color the mode indicator with different colors depending on the mode itself | `false` |
| `centered-views` | Wether to center views by default | `false` |
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| `centered-views` | Wether to center views by default | `false` |
| `centered-views` | Whether to center views by default | `false` |

@@ -405,6 +405,7 @@ impl MappableCommand {
wonly, "Close windows except current",
select_register, "Select register",
insert_register, "Insert register",
toggle_centered_view, "Toggle centered view",
Copy link
Member

Choose a reason for hiding this comment

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

The naming seems confusing since the view isn't "centered" but is instead in a distraction free mode. Tmux also calls this "zoomed (in)" since the view is maximized and replaces the whole tree.

@@ -138,7 +139,7 @@ impl fmt::Debug for View {
}

impl View {
pub fn new(doc: DocumentId, gutters: GutterConfig) -> Self {
pub fn new(doc: DocumentId, gutters: GutterConfig, is_centered: bool) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

The default should be false and we don't want this as a part of the new() method.

Comment on lines +277 to +278
/// Whether to center views by default, Defaults to `false`.
pub centered_views: bool,
Copy link
Member

Choose a reason for hiding this comment

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

I'd remove the config option since it seems unlikely someone would want this always on.

Comment on lines +363 to +373
if view.is_centered
&& (parent_child_count <= 1 || parent_layout != Layout::Vertical)
{
let width = std::cmp::min(std::cmp::max(area.width / 2, 120), area.width);
let area =
Rect::new(area.width / 2 - width / 2, area.y, width, area.height);

view.area = area;
} else {
view.area = area;
}
Copy link
Member

Choose a reason for hiding this comment

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

This gives the view the entire area but doesn't affect any of the other views. This means that you're relying on view ordering (this view should be the last one rendered) otherwise other views might get rendered over it. We also pay the performance cost of rendering all other views even though they're not on screen. (Also what happens if I change focus to a different view but this one is still centered?)

I'd rather see this done via a zoom bool flag on the tree. If set to true then the editor should just render the currently focused node and ignore the rest of the tree. The reason we haven't done this yet is because this will likely impact a bunch of other viewport calculations.

Copy link
Author

Choose a reason for hiding this comment

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

Hi, thanks for the feedback, but... to be honest I made this PR 7 months ago, so I don't remember at all why I made things in this way, and I don't have sufficent spare time to invest in this PR anymore :/ if someone is motivated enough to take over my work (or redoing it from scratch ^^') it would be better

@ShalokShalom ShalokShalom mentioned this pull request Mar 9, 2023
Closed
@CBenoit CBenoit added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Mar 10, 2023
@David-Else
Copy link
Contributor

It would be great if someone could take over this pull request, it is so needed!

@the-mikedavis the-mikedavis added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 10, 2024
@pascalkuthe
Copy link
Member

closing in favor of #9838

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Centered code mode (like intellij's distraction free mode)
9 participants