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

Custom file picker width max min and factor #1089

Conversation

Rational-Curiosity
Copy link
Contributor

@Rational-Curiosity Rational-Curiosity commented Nov 12, 2021

This allow custom file picker width settings

@Omnikar
Copy link
Contributor

Omnikar commented Nov 13, 2021

This isn't a percentage.

@Rational-Curiosity Rational-Curiosity changed the title Custom file picker percentage width Custom file picker width absolute and factor Nov 13, 2021
@Rational-Curiosity
Copy link
Contributor Author

This isn't a percentage.

I have removed the word 'percentage' and expanded the meaning of the variable.

@@ -23,6 +23,7 @@ To override global configuration parameters, create a `config.toml` file located
| `idle-timeout` | Time in milliseconds since last keypress before idle timers trigger. Used for autocompletion, set to 0 for instant. | `400` |
| `completion-trigger-len` | The min-length of word under cursor to trigger autocompletion | `2` |
| `auto-info` | Whether to display infoboxes | `true` |
| `file-picker-width` | File picker width, values from 0.0 to 1.0 treated as a factor | `0.5` |
Copy link
Member

Choose a reason for hiding this comment

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

There's going to be an editor.file-picker section (#988) so this can be a key in that sub config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, then we have to wait #988 to be merged

@@ -166,7 +166,12 @@ impl<T: 'static> Component for FilePicker<T> {
surface.clear_with(area, background);

let picker_width = if render_preview {
Copy link
Member

Choose a reason for hiding this comment

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

It would be less confusing to have the config as preview-width instead of picker-width since I would assume picker-width to mean the width of the entire window (including the preview and the picker).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But preview is the right side not always showed. Maybe we have to choose a name for the left side of the picker. Suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

Then I think we can have two different options - file-picker.width and file-picker.preview-width. width can control the whole picker + preview window size and preview-width can control the size of the preview inside this window.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :-)

@pickfire
Copy link
Contributor

pickfire commented Nov 14, 2021

I wonder if this could be represented using both percentage and/or min width? How do we factor those in later?

@Rational-Curiosity
Copy link
Contributor Author

Rational-Curiosity commented Nov 14, 2021

I wonder if this could be represented using both percentage and/or min width? How do we factor those in later?

Percentage and factor represents the same thing 50% = 0.5, and the current implementation allows both proportional part of the picker width or number of characters with only one variable (similar to how emacs do it). Min width is very interesting, any suggestion about how to name it?

@Rational-Curiosity Rational-Curiosity force-pushed the file-picker-perc-width branch 2 times, most recently from bf4290a to 61be1fd Compare November 22, 2021 21:15
helix-view/src/editor.rs Outdated Show resolved Hide resolved
@Rational-Curiosity Rational-Curiosity changed the title Custom file picker width absolute and factor Custom file picker width max min and factor Nov 23, 2021
Copy link
Contributor

@pickfire pickfire left a comment

Choose a reason for hiding this comment

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

Didn't get to test this out but code wise looks good to me. Just some comments left.

|`height-factor` | File picker height, treated as a factor. | `0.9` |
|`preview-width-factor` | File picker preview width, treated as a factor. | `0.5` |
|`preview-min-width` | File picker preview minimum width in columns. | `60` |
|`file-min-width` | File picker filenames minimum width in columns when preview is showed. | `20` |
Copy link
Contributor

Choose a reason for hiding this comment

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

filenames minimum width seemed confusing to me?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i know my English sucks ;-), any suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

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

File picker preview minimum width in columns?

Copy link
Contributor Author

@Rational-Curiosity Rational-Curiosity Nov 25, 2021

Choose a reason for hiding this comment

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

+----------------+-------------------+
| file-min-width | preview-min-width |
| filenames...   | code...           |
+----------------+-------------------+

file-min-width is the other side.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how to explain this too, hopefully someone can help.

Comment on lines +87 to +91
preview_min_width: 60,
file_min_width: 20,
Copy link
Contributor

@pickfire pickfire Nov 24, 2021

Choose a reason for hiding this comment

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

We should add a comment that both of this should total up to 80.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Out of curiosity, what is the reason? preview-min-width = 25, file-min-width = 20 works, and preview-min-width = 80, file-min-width = 30 works too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought that's why you put 80? Like previously I saw a number 80 being removed, and you replaced it with the sum of these two.

At the very least I think we should still make it look nice for 80x24 terminals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :-)

Copy link
Contributor

@pickfire pickfire left a comment

Choose a reason for hiding this comment

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

Tried locally and didn't seemed to break current behavior so should be good.

Comment on lines +164 to +165
cx.editor.config.file_picker.width_factor,
cx.editor.config.file_picker.height_factor,
Copy link
Member

Choose a reason for hiding this comment

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

Pass in &cx.editor.config.file_picker instead

Comment on lines +168 to +169
> cx.editor.config.file_picker.preview_min_width
+ cx.editor.config.file_picker.file_min_width;
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit repetitive, add let config = &cx.editor.config.file_picker at the top of render

Comment on lines +180 to +187
cmp::max(
area.width
- cmp::max(
preview_width,
cx.editor.config.file_picker.preview_min_width,
),
cx.editor.config.file_picker.file_min_width,
)
Copy link
Member

Choose a reason for hiding this comment

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

Rather than cmp::max(a, b) a.max(b) can be used. In this case a.max(b).max(c).

Comment on lines +409 to +410
vertical: (area.height as f32 * (1.0 - width_factor)).round() as u16,
horizontal: (area.width as f32 * (1.0 - height_factor)).round() as u16,
Copy link
Member

Choose a reason for hiding this comment

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

Using floats feels wrong to me, why not specify a percentage from 0-100?

@the-mikedavis the-mikedavis added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 18, 2022
@kirawi
Copy link
Member

kirawi commented Sep 13, 2022

Closing as inactive. Feel free to open a new PR if you wish to continue.

@kirawi kirawi closed this Sep 13, 2022
@kirawi kirawi 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 Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

7 participants