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

feat: add configurable window shade callback #22

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yujinyuz
Copy link
Contributor

@yujinyuz yujinyuz commented Jul 6, 2024

Closes #20

@yujinyuz
Copy link
Contributor Author

yujinyuz commented Jul 6, 2024

@miversen33 Still need to update the documentation.

Let me know what you think about the function callback name. I couldn't think of anything better atm.

local diff_mode = vim.api.nvim_get_option_value('diff', { win = self.window })

if diff_mode then
return "currently in diff mode"
Copy link
Owner

Choose a reason for hiding this comment

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

There are a couple issues with this.

  1. By placing this here, you are short circuiting the filetype and buffertype exclusions below. This means that you are saying "yes we are in diff mode" and shade will accept that as a valid state and shade it regardless of any of the below logic.
  2. This should be configurable. Users may not (not likely but still) care about diff mode and want it shaded anyway. The current logic here will not allow that. There should be a check to see if the user wants to shade diffs. I do agree (as stated in Auto disable when buffer is diff #20 ) that it should be enabled by default, but still the option to revert should be there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense.

What if we include the logic for auto-disabling the diff within the callback function so that the user can just pass their own callback if they want to override it?

Copy link
Owner

Choose a reason for hiding this comment

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

I would move this section to between 144 and 146. And probably change it to be something like

    if vim.api.nvim_get_option_value('diff', { win = self.window }) and self.can_shade_diff then
        -- Not sold on this either, but as called out in option 2, this should be configurable
        return "currently in diff mode"
    end

This would rely on having some sort of configuration option on the window that states that it can shade diff mode but still allows the filetype and buftype exclusions to operate as expected.

Note, I have not tried the above code, but hopefully it gives a sense of where I see that going

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense but I was thinking of maybe just moving that logic within defaults.lua to make things simpler

    -- defaults.lua
    ...
    window_can_shade_callback = function(winid)
      local diff_mode = vim.api.nvim_get_option_value('diff', { win = winid })

      if diff_mode then
        return "currently in diff mode"
      end

    return true
    end,

Copy link
Owner

Choose a reason for hiding this comment

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

TLDR: I don't think diff logic alone should be part of this function


The trade off being that if a user supplies their own "window_can_shade_callback", that they lose the ability to auto shade diffs (without implementing it themselves)?

I don't hate that idea, though it does provide some inconsistency (in its current implentation). Where we do this right now, the user would lose the ability to have sunglasses auto shade diff windows but not before sunglasses potentially already decided based on filetype/buftype.

I don't hate this function but I see it filling one of 2 purposes.

  1. It is a fallback that is called after all core processing is complete (including diff logic)
    Effectively this would allow a user to say "if sunglasses didn't shade the window but I still want to have some custom logic to do so, now I can"

  2. It is the primary logic if supplied
    This means that if this function was provided by the user, we call it immediately in Window:can_shade and forgo all core processing logic. The user wants to determine how a window is shaded, they have to manage that entire logic flow

In either case, diff should be part of core and thus not part of this function (unless option 2 is chosen and the user decides to handle it).

@miversen33 miversen33 self-assigned this Jul 7, 2024
@miversen33
Copy link
Owner

@yujinyuz are you still working on this? If not, I am going to close the PR out

@yujinyuz
Copy link
Contributor Author

@miversen33 I'm not actively working on it since I don't have enough bandwidth.

I'm just using this PR's version in my local since it works for my use case.

Feel free to close this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto disable when buffer is diff
2 participants