Update display config methods

[l-kershaw]

Plan:

• Implement hierarchy of options that affect window size. (requires fuller testing)
• Remove width_padding and height_padding option from layout strategies.
• Add new option for width and height allowing a table of the form {padding=foo} where foo is one of the other options, and then define width in terms of padding.
• Add documentation for the new method of defining dimensions of the picker.

For more details on the general idea, see the discussion in this issue.

---

Feedback is very welcome 🙂

[Conni2461]

Thanks for all the work here. I know that tj also had plans to change how layouts work. So its hard for me to comment on your proposal. I know there is this: #773 and #771

Its definitely one of the areas telescope needs improvement. So thanks again.

PS: Ignore tests, #821 is currently breaking everything. But make sure that your fork has github actions enabled (or permissions) so docgen runs. It updates the vim documentation based on function headers. :)

[tjdevries]

This looks cool, but wait a week for me to take a look when I'm back before merging. It's OK if we make some breaking changes, but I'd prefer to only break it once for a bit 😂

Thanks for the PR, I like where this is going.

[l-kershaw]

Couple of comments on defaults affecting layout:

• I think we should rename layout_defaults to layout_config
  • Since this option is already contained in the default block of options in setup(), I don't think repeating the word default is necessary
  • This means that the key used for these matches in setting defaults and in setting options for a particular command
• I think that the defaults that we have for layout are good in that it's clear how they work, but they probably aren't the best default options in terms of usability.
  • For example, for particularly thin windows, it would be better to define width in terms of padding, but for wider windows it's better to use a percentage.
  • We could change the defaults to be functions that take into account the size of the Neovim window, which I think would make them better for users, but probably harder to understand and/or document.

What are people's thoughts on this?

[smithbm2316]

• I think we should rename layout_defaults to layout_config
  • Since this option is already contained in the default block of options in setup(), I don't think repeating the word default is necessary
  • This means that the key used for these matches in setting defaults and in setting options for a particular command

I think this is a good idea. I was trying to debug an issue recently with the layout_config causing an issue with the flex layout, and it was a bit confusing to me bouncing back and forth between places that said layout_defaults and layout_config. I think one name would make it much simpler.

• I think that the defaults that we have for layout are good in that it's clear how they work, but they probably aren't the best default options in terms of usability.
  • For example, for particularly thin windows, it would be better to define width in terms of padding, but for wider windows it's better to use a percentage.
  • We could change the defaults to be functions that take into account the size of the Neovim window, which I think would make them better for users, but probably harder to understand and/or document.

I like the sound of this, but don't really know enough about how the size of Telescope is currently calculated to have a very meaningful opinion on this aspect. I'll let others hopefully give more constructive feedback on this point.

Thanks for all of your hard work lately @l-kershaw, really looking forward to seeing some of the code you've written getting merged!

[l-kershaw]

Yeh, I was having the same issue. I have changed the name so it matches now

---

I like the sound of this, but don't really know enough about how the size of Telescope is currently calculated to have a very meaningful opinion on this aspect. I'll let others hopefully give more constructive feedback on this point.

I think this is fairly common, particularly as some layout strategies ignore some options that it seems like they shouldn't.
E.g. on the current master branch, the horizontal layout ignores the width option, and only changes based on the width_padding option within layout_config. If this is not present, it has a hard-coded option within the function, rather than being set as a default with the rest of the defaults. This function does depend on the width of the window, but it is not clear how unless you dig into the code.

Hopefully making it clearer how this is calculated will make it easier for users to customise as they wish.
That's the aim of this PR 🙂

[tjdevries]

I think the tests should not be necessariyl about the "does the config load correctly" but "do we merge the tables".

We can export a function _merge_config_tables that doesn't require any global state to check the results.

I will see if I can do anything related to this today.

[tjdevries]

Some stuff I'd like to see done before this PR is merged:

• Update readme. I want readme to have less in it and point more to the appropriate help sections (e.g. :help telescope.layouts or whatever.) This is because we don't auto-generate the readme, but we can autogenerate the docs. And I only want one place of truth for the layout information as much as possible (some stuff is fine to stay cause it gives people an idea of what they can do with telescope in the readme).
• I think I want to see the telescope_defaults look more like this, rather than the current way.

local telescope_defaults = {
  sorting_strategy = { ... }
}

• I think preview_cutoff doesn't make sense the way it is currently. So, center should not do cutoff based on columns, but rather on rows, right?

@l-kershaw what else do you think?

[l-kershaw]

Re: your first comment

• I'm not sure that checking if we merged the config tables correctly is sufficient. We should check if we are correctly interpreting the user provided override (the config passed when the Telescope is called, after it has been setup).
  We can add some tests for checking the merging explicitly as well if we want, although I'm not sure it is necessary as this is sort of inherently checked with the current tests.

Re: your second comment

• I agree with removing some stuff from the readme and pointing to the docs instead. This will definitely make it easier for us to update stuff with less confusion in future.
• I'm a bit confused by your second point. I think you mean that we should have the default layout_config separated by layout_strategy, which is definitely something we can do in the scope of this PR.
  If this isn't what you mean, could you please clarify?
• Regarding preview_cutoff we could change this to a resolvable setting where users could input things like {columns=120}, {rows=50} or just a function that takes as inputs the maximum number of columns and rows and returns a boolean. We could even allow just a number and keep the current functionality for columns if we wanted to implement this change with minimal breaking of configs (although this would add complexity that I'm not sure is necessary).
  This would then allow us to set the center layout to have a cutoff in terms of rows, for example.

I can't think of any additional aims for this PR atm, but happy to discuss if anyone else has suggestions.

[l-kershaw]

I have done a first pass on implementing the ability to resolve preview_cutoff, which allows us to have cutoffs in terms of the number of lines in the window and things like that.

@tjdevries could you have a look to see if this is the sort of thing that you wanted?

[tjdevries]

So for preview cutoff, I was thinking we could just have different help text for vertical vs. horizontal vs. center (etc.). It would just say that it does rows in one and columns in the other. I don't think we need to make a completely new style of resolver or anythinga

[tjdevries]

(I'll respond to other comments later)

[Conni2461]

Default theme in setup was suggested yesterday after i merged picker configuration. See #883 (comment)

Could be part of this PR as well. Thoughts?

[l-kershaw]

Not sure why the linter is complaining as I can't find any lines that are too long in the file specified 🤷

---

@tjdevries I've reverted the resolve setup and switched to comparing for lines with the center layout (and added a check for lines with the vertical layout too`). Is this more what you were thinking?

@Conni2461 I like the idea, but I'm not sure if it should be part of this PR. It doesn't rely on any of the new stuff here, or vice versa, so IMO it should be a separate PR. If people have strong preferences for it being in this one we can do, I'm just a bit concerned about scope creep with this PR, as it's already quite big 😅

[Conni2461]

You can rebase in ~5 min again. Then linting should be fixed. I merged something with a lint error (i missed it because of docgen) but the same person had another PR open that was almost done and linting will be addressed there. I will merge it after i have tested it.

Yeah separate PR for that theme thing is probably better. We wanted smaller PRs now anyway. 🤣

Edit: you can rebase if you want to get rid of the linting error :)

[l-kershaw]

I can't figure out why, but the help tags between action_state.get_current_picker() and resolver.win_option don't seem to be working properly. Neovim doesn't seem to think they exist, despite items on either side of them in doc/telescope.txt working fine.

Possibly something to do with running out of space on the action_state.get_current_picker() line???
I'm not familiar with all the nitty gritty of how vim help files are processed, so not really sure.

Any ideas?

[tjdevries]

Let's do theme in a different PR -- It shouldn't be a breaking change, right? So we can do that part later? I don't want to add anything else to this if we can help it.

[tjdevries]

OK, I think this is very very close to merging -- but I don't want to merge before I'm gone for a week in case something goes weird. So maybe we wait until after that and then we can merge when I'll be around

[Conni2461]

replaced by #922

Thanks for your work @l-kershaw