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

Fix state/config loading, test on all PRs and add use case #115

Merged
merged 12 commits into from
Mar 11, 2024

Conversation

5-pebbles
Copy link
Collaborator

@5-pebbles 5-pebbles commented Feb 29, 2024

This PR improves the handling of state and config loading.

@5-pebbles 5-pebbles mentioned this pull request Feb 29, 2024
@5-pebbles 5-pebbles requested a review from AlexvZyl February 29, 2024 16:40
@AlexvZyl AlexvZyl changed the base branch from main to fix/state February 29, 2024 17:40
@AlexvZyl
Copy link
Owner

You can change the message if you want, but we are merging this into #113.

@5-pebbles
Copy link
Collaborator Author

sorry, I misread that message.

@5-pebbles
Copy link
Collaborator Author

I resolved conflicts then ran tests locally with act and they are passing.

I figure I should let you merge though if this is what you are looking for.

@AlexvZyl
Copy link
Owner

No longer need to run the tests locally! :)

@5-pebbles
Copy link
Collaborator Author

Thanks.

@AlexvZyl
Copy link
Owner

Did you see I started a review?

@5-pebbles
Copy link
Collaborator Author

No, I did not. Quite frankly this is the first project I have tried actually using GitHub for so I don't know any of this stuff.

@5-pebbles
Copy link
Collaborator Author

Yeah, I have no idea what I am supposed to be doing.

@AlexvZyl
Copy link
Owner

AlexvZyl commented Feb 29, 2024

No worries! One step at a time.

I left comments (in this PR UI) on the changes you made. Try and find them and respond to them.

@5-pebbles
Copy link
Collaborator Author

5-pebbles commented Feb 29, 2024

I though comments showed up in this chat, but I don't see any.
I also though they showed up on the Files changed tab but I don't see anything there either.
And it still says pending reviewer.

@5-pebbles
Copy link
Collaborator Author

Ok, I can not see the code review I tested it on a private repo and it should be here but its not.

@AlexvZyl
Copy link
Owner

Scroll through the "files changed" tab.

@AlexvZyl
Copy link
Owner

Odd. I will check it out tomorrow.

@5-pebbles
Copy link
Collaborator Author

There are no comments there.

@5-pebbles
Copy link
Collaborator Author

Alright Good Night!

@AlexvZyl
Copy link
Owner

AlexvZyl commented Mar 1, 2024

@5-pebbles You should be able to see the review now.

@5-pebbles
Copy link
Collaborator Author

Yup I can.

@5-pebbles
Copy link
Collaborator Author

5-pebbles commented Mar 1, 2024

I am thinking about separating bg_highlight and bg_visual.

I think bg_highlight makes sense for Search and CodeBlock, plus it was only used into places:

  • G.CursorColumn & G.CursorLine

which I would move to using bg_visual

Its value would look something like this.

C.bg_highlight = (options.swap_backgrounds and C.gray0) or C.black1
-- so same as bg_float but without transparency

What do you think?

@5-pebbles
Copy link
Collaborator Author

I don't know why its not showing up in the pr but 4856cf8 contains the proposed changes.

@5-pebbles
Copy link
Collaborator Author

I re-selected fix/state as base to force sync.

@AlexvZyl
Copy link
Owner

AlexvZyl commented Mar 1, 2024

Sure, I think that will work!

@AlexvZyl AlexvZyl changed the title Fix: State and Config Reload Fix state, test on all PRs and add another use case Mar 1, 2024
@AlexvZyl
Copy link
Owner

AlexvZyl commented Mar 1, 2024

I think we need a better name than bg_highlight. bg_highlight does not make sense for the search bar...

Maybe something like bg_bar? What do you think?

@AlexvZyl AlexvZyl changed the title Fix state, test on all PRs and add another use case Fix state/config loading, test on all PRs and add use case Mar 1, 2024
@5-pebbles
Copy link
Collaborator Author

I don't know I think bg_highlight is good, its the a highlighted section of background, eg a search item or code block.

But naming this is really not my forte so I will leave that up to you, but I used it for code blocks as well and bg_bar does not sound right for those.

@AlexvZyl
Copy link
Owner

AlexvZyl commented Mar 2, 2024

Hmm. I don't think we should use black0 for code blocks. Rather black1 or maybe black2.

@5-pebbles
Copy link
Collaborator Author

Agreed and that is already the case:

G.CodeBlock = { bg = C.bg_highlight, fg = C.fg }

C.bg_highlight = (options.swap_backgrounds and C.gray0) or C.black1

@AlexvZyl
Copy link
Owner

AlexvZyl commented Mar 3, 2024

Ah okay I see, but search should be black0.

@AlexvZyl
Copy link
Owner

AlexvZyl commented Mar 3, 2024

Actually, we need bg_statusline and Search should use that.

@5-pebbles
Copy link
Collaborator Author

Alright I will make a commit for that, but it was black1 before and I was just following along with that.

@5-pebbles
Copy link
Collaborator Author

So bg_statusline is none when transparent background is true do you want no search highlight?

What about using bg_visual its the black0 though I would like to make it customizable with something like #100. It might make sense for search to be the same color as visual section.

I also thought it might make sense to have transparent code blocks and I could add those in the same commit.

@5-pebbles
Copy link
Collaborator Author

What do you think about my last comment ^ @AlexvZyl

@AlexvZyl
Copy link
Owner

Sorry @5-pebbles!

bg_statusline should not be NONE when transparent is enabled.

@AlexvZyl
Copy link
Owner

AlexvZyl commented Mar 10, 2024

Also, with customizability, I want to add two callbacks:

  1. To override the base palette before extension.
  2. To override the extended colors as well.

But we should do this in a seperate PR.

@AlexvZyl
Copy link
Owner

The reason I want search to be black0 is because search happens in the bottom bar, so should be black0.

@5-pebbles
Copy link
Collaborator Author

5-pebbles commented Mar 10, 2024

Sorry @5-pebbles!

bg_statusline should not be NONE when transparent is enabled.

Aright I will fix that, because it is NONE on main.

The reason I want search to be black0 is because search happens in the bottom bar, so should be black0.

To make sure we are both on the same page the search highlight is:
10-03-24@15:44:28
10-03-24@15:45:35

I will set it to black0 but its set to black1 on main.
I think it would make sense for it to be bg_visual but I can do that in my own config.

Also would it make sense for code blocks to be transparent with transparent_bg?
I think it would... and if so I think they should be bg_float they would have the same values as on main just with transparency.

I will get to these tomorrow because I have a bunch of other stuff to do today.

@AlexvZyl
Copy link
Owner

Okay, this entire time I though the search highlight was that highlight that colors the bottom bar, where you type in the search. The bottom bar should be black0, the ones you are referring to should either be a lighter black or some grey, which ever you think looks best.

As for the code blocks being transparent, I am not sure... I think they should be.

@5-pebbles
Copy link
Collaborator Author

Okay, this entire time I though the search highlight was that highlight that colors the bottom bar, where you type in the search. The bottom bar should be black0, the ones you are referring to should either be a lighter black or some grey, which ever you think looks best.

I think they should be bg_visual same as the visual selection (that way they will also change with the light and dark cursorline setting).

As for the code blocks being transparent, I am not sure... I think they should be.

Alright great because I already did that...

`bg_statusline` and `bg_visual` are no longer set to none with transparent
bg.

`Search` now uses `bg_visual`
& `CodeBlock` is now transparent with `transparent_bg`
@5-pebbles
Copy link
Collaborator Author

If you are okay with those changes I think everything in this pr is done.

@AlexvZyl
Copy link
Owner

Cool!

@AlexvZyl AlexvZyl merged commit ad0da11 into fix/state Mar 11, 2024
1 check passed
@5-pebbles 5-pebbles deleted the state_and_reload branch March 24, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature, request or suggestion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No selection highlight when transparent mode is enabled swap_backgrounds not working
2 participants