Skip to content

PopoverManager: DRY and organize current_indicator setter - #734

Open
danirabbit wants to merge 4 commits into
mainfrom
danirabbit/popovermanager-currentdryorganize
Open

danirabbit wants to merge 4 commits into
mainfrom
danirabbit/popovermanager-currentdryorganize

Conversation

@danirabbit

Copy link
Copy Markdown
Member
  • Remove some else/if nesting where possible
  • DRY things that happen on both close and switch
  • DRY things that happen on both switch and open
  • Don't unparent in popover close because we do that already in current_indicator = null

@danirabbit
danirabbit requested review from a team and lenemter September 19, 2026 17:50
@lenemter
lenemter force-pushed the danirabbit/popovermanager-currentdryorganize branch from 4e9fa7f to 2f549f3 Compare October 2, 2026 16:44

@lenemter lenemter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The indicators randomly stop opening for me until I reopen Applications menu through via the shortcut

}

set {
// Double close. Shouldn't happen?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this comment is needed

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I thought it was worth explaining why we return here

@danirabbit

danirabbit commented Oct 2, 2026 •

Copy link
Copy Markdown
Member Author

@lenemter that happens to me in main as well. There shouldn't be any logic changes in this branch, just refactoring

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.

2 participants