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 completion.item_padding #7292

Closed
wants to merge 2 commits into from
Closed

Add completion.item_padding #7292

wants to merge 2 commits into from

Conversation

kyza0d
Copy link

@kyza0d kyza0d commented Jul 2, 2022

No description provided.

Copy link
Member

@The-Compiler The-Compiler left a comment

Choose a reason for hiding this comment

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

This just crashes qutebrowser outright. Please make sure you actually try your code and can confirm it works.

qutebrowser/completion/completionwidget.py Outdated Show resolved Hide resolved
Pull request backlog automation moved this from Inbox to WIP Jul 2, 2022
Co-authored-by: Florian Bruhin <me@the-compiler.org>
@The-Compiler The-Compiler moved this from WIP to Inbox in Pull request backlog Jul 3, 2022
@segf00lt
Copy link

segf00lt commented Jul 3, 2022

For controling the padding values it might be better to use separate options instead of a Dict.

completion.item_padding.top:
  default: 1
  type:
    name: Int
    minval: 0
  desc: Top padding (in pixels) of items in the completion window.

completion.item_padding.bottom:
  default: 1
  type:
    name: Int
    minval: 0
  desc: Bottom padding (in pixels) of items in the completion window.

completion.item_padding.left:
  default: 0
  type:
    name: Int
    minval: 0
  desc: Left padding (in pixels) of items in the completion window.

completion.item_padding.right:
  default: 0
  type:
    name: Int
    minval: 0
  desc: Right padding (in pixels) of items in the completion window.

And the changes for the stylesheet in completionwidget.py:

        QTreeView:item {
            padding-top: {{ conf.completion.item_padding.top }}px;
            padding-bottom: {{ conf.completion.item_padding.bottom }}px;
            padding-left: {{ conf.completion.item_padding.left }}px;
            padding-right: {{ conf.completion.item_padding.right }}px;
        }

@The-Compiler
Copy link
Member

Why? We already have a Padding type we use elsewhere, so it seems to make sense to use it here too, no?

@segf00lt
Copy link

segf00lt commented Jul 3, 2022

Yeah that makes more sense. I was thinking it would be a setting the user would occasionally change at runtime, but just as a setting in the config file it does make more sense to use Padding.

@vereis
Copy link

vereis commented Feb 22, 2023

This looks like it hasn't had any updates in awhile, @The-Compiler would you be open to another shot at implementing this feature?

I'm not sure what the process typically is when it comes to these PRs which have been sitting around for awhile?

@The-Compiler
Copy link
Member

@vereis Sorry for the delay, I was taking a bit of a break from qutebrowser stuff. I think this one should actually be okay from a quick look, unless you see something that would be missing here.

Usually with older PRs it's really appreciated if someone helps picking things up and getting them updated, but here I think there is nothing left to do, except on my end of things. I'm currently focusing on Qt 6 stuff, but then want to go back and hopefully merge a big batch of PRs again after that.

Copy link
Contributor

@mschilli87 mschilli87 left a comment

Choose a reason for hiding this comment

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

configdata.yml changes LGTM as well.

@vereis
Copy link

vereis commented Mar 13, 2023

No worries at all! I really appreciate it; I've only started using qutebrowser recently but I'm really loving it so far!

Really appreciate you following up again ❤️ Looking forward to everything else you have in store!

top: 1
bottom: 1
left: 0
right: 0
Copy link
Member

Choose a reason for hiding this comment

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

I somewhat prefer the old default of 0 for all

@kyza0d kyza0d closed this by deleting the head repository Jan 2, 2024
Pull request backlog automation moved this from Inbox to Done Jan 2, 2024
@mschilli87
Copy link
Contributor

@The-Compiler: Just making sure this doesn't slip through your attention due to @kyza0dev removing their fork.

@arza-zara arza-zara moved this from Done to Inbox in Pull request backlog Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants