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

Display Configuration #818

Closed
l-kershaw opened this issue May 5, 2021 · 3 comments
Closed

Display Configuration #818

l-kershaw opened this issue May 5, 2021 · 3 comments
Labels
enhancement Enhancement to performance, inner workings or existent features

Comments

@l-kershaw
Copy link
Contributor

Will talk about width, but same logic applies to height.

Is your feature request related to a problem? Please describe.

Two different variables are used to calculate the width of a picker

  • width
    • a top level option/default
    • default value: 0.75
  • width_padding
    • an entry in the layout_defaults option/default
      • allows you to specify a default value directly within layout_defaults
      • also allows you to specify a custom value for each layout_strategy
    • default value: hard-coded in each strategy

Problems:

  • These values can be calculated from each other by width_padding = math.floor((max_columns - width)/2) or width = max_columns - 2 * width_padding
  • Therefore most of the layout strategies ignore the width option and just use the width_padding, then calculating the appropriate width.
  • This can make it confusing to users, as you would expect changing the width option to affect the width of the picker.

Describe the solution you'd like

A hierarchy of options/defaults.
When calculating the width of a picker with strategy strat, use the first non-null value in the following list to calculate both the width and the width_padding.

  • opt.width
  • opt.layout_config[strat].width
  • opt.layout_config[strat].width_padding
  • opt.layout_config.width
  • opt.layout_config.width_padding
  • default.width : default value=nil
  • default.layout_config[strat].width : default value=nil
  • default.layout_config[strat].width_padding : default value=nil
  • default.layout_config.width : default value=nil
  • default.layout_config.width_padding : default value=math.max(max_columns*0.1,5)

Advantages:

  • Since width is still a top-level option, you can specify a custom width of the picker easily in any custom mappings.
  • Since there is now a pre-defined order, you know that if you specify a width for a picker, it will actually affect the width, regardless of the layout.

Disadvantages:

  • This is a lot of options that only affect one thing.
    • Like, a lot, a lot.

Describe alternatives you've considered

  • Remove the width_padding options/defaults entirely.
    • Since width_padding can be calculated easily from width, this vastly simplifies the code required.
    • This would be a breaking change for some people's configs, especially as it affects the more commonly used layout strategies.
    • This makes it slightly more complicated for a user to specify that they want an n column gap on each side of the picker.
  • Remove the width options/defaults entirely.
    • Again, since width can be calculated easily from width_padding, this simplifies the code required.
    • This would be a breaking change for some people's configs, but only for those who rely on the center layout strategy, as the other strategies don't check this option.
    • This makes it more complicated for a user to specify that they want the picker to take up a fixed with on the screen.

If we had to pick one of these alternatives, I would remove width_padding as the notion of width is clearer to the user.

Additional context

I'm happy to have a go at putting a PR for this together, but wanted to get feedback on the general idea first.

@l-kershaw l-kershaw added the enhancement Enhancement to performance, inner workings or existent features label May 5, 2021
@smithbm2316
Copy link
Contributor

This is one of the aspects of Telescope that still definitely needs some improving. I think you've got some good ideas here from an initial read, and I'd love to see what kind of PR you can put together. My initial gut reaction is that this could end up getting quite complicated with so many options, but I'll have to think on it for a day or two before I have any kind of valuable feedback to offer. Thanks for the suggestion and plan of action!

@l-kershaw
Copy link
Contributor Author

I have looked more into how this sort of thing could be implemented, and I think it would be particularly difficult to have this order for the hierarchy because of the way that picker.new is defined here.
The reason for this is that the "use the option provided" vs. "use the default" processing is done in the picker.new function, before the layout strategy is called.

Without rewriting this function to handle a load of special cases, we could implement a different hierarchy that still has most of the benefits.
Specifically we would end up with an "interleaved" ordering along these lines:

  • opt.width
  • default.width : default value=nil
  • opt.layout_config[strat].width
  • default.layout_config[strat].width : default value=nil
  • opt.layout_config[strat].width_padding
  • default.layout_config[strat].width_padding : default value=nil
  • opt.layout_config.width
  • default.layout_config.width : default value=nil
  • opt.layout_config.width_padding
  • default.layout_config.width_padding : default value=math.max(max_columns*0.1,5)

I also think that we can remove the width_padding options in a way that still allows users to easily define things in terms of distance from the edge. This can be done by adding another case to _resolve_map here.
If we allow users to input a table with a key of padding and a value that is then processed in the same way as other cases, we can then convert the output to width using the formula mentioned above.

This then leaves us with a hierarchy like this:

  • opt.width
  • default.width : default value=nil
  • opt.layout_config[strat].width
  • default.layout_config[strat].width : default value=nil
  • opt.layout_config.width
  • default.layout_config.width : default value=0.8

This seems like a reasonable compromise of much fewer options to deal with, but still allowing the user to give options in a form that is more convenient to them.

With this system, we still have the ability to give width as a top level option (and are able to define it in terms of padding if we want) and we can have a predefined order of precedence that the layout strategies know how to deal with.

The downside of this new "interleaved" hierarchy is that if a user defines the width as a top level option in their defaults, it could be very confusing when they input an option lower on the ordering and it has no effect. We could mitigate this a bit by making this ordering clear in documentation.


Unless there are any objections to this setup, I'll have a go at making a PR for this over the next couple of days.

@Conni2461
Copy link
Member

good job @l-kershaw 5a53ec5 :)

Closing for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to performance, inner workings or existent features
Projects
None yet
Development

No branches or pull requests

3 participants