Skip to content

feat(xrandr): comma separated --setmonitor third argument #1026

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

Merged
merged 1 commit into from
Aug 13, 2023

Conversation

scop
Copy link
Owner

@scop scop commented Aug 6, 2023

Requires/stacks on top of #1025, drafting until that one is merged.

@scop scop marked this pull request as draft August 6, 2023 08:34
@scop scop force-pushed the refactor/more-api-naming branch from 4d5be91 to 0b98335 Compare August 6, 2023 11:58
@akinomyoga
Copy link
Collaborator

akinomyoga commented Aug 6, 2023

Requires/stacks on top of #1025,

It seems most of the comments I made here should have been put in PR #1025. Sorry for the noise, but could you apply these to PR #1026?

@akinomyoga akinomyoga force-pushed the refactor/more-api-naming branch from f89c328 to d0695d0 Compare August 12, 2023 02:38
Base automatically changed from refactor/more-api-naming to master August 13, 2023 08:59
@scop scop force-pushed the feat/xrandr-setmonitor-3rd branch from e2dfc91 to 61577cc Compare August 13, 2023 10:03
@scop scop marked this pull request as ready for review August 13, 2023 10:09
@scop
Copy link
Owner Author

scop commented Aug 13, 2023

Requires/stacks on top of #1025,

It seems most of the comments I made here should have been put in PR #1025. Sorry for the noise, but could you apply these to PR #1026?

Hmm, #1026 is this one, and #1025 was merged an hour or so, without the suggestions that are still open here. I've rebased this on top of master, reducing it to just the xrandr changes it should have contained in the first place.

Some of the suggestions were moved over to #1028, but the ones still open here are not in master nor #1028. They don't really belong here, should they be moved to #1028 instead? At least they're related.

@akinomyoga
Copy link
Collaborator

Thanks. I'll move them to #1028 with the approach of -U var.

These (and #1028) are related to the variable name conflicts with nested generators. There are two solutions:

  1. One is to use the local _tmp=$tmp; _comp_unlocal tmp; _comp_compgen ... '$_tmp' pattern, but it is tedious to write them every time and it doesn't still solve the problem perfectly as it can still conflict with underscored variables in the function _comp_compgen itself.
  2. The other is -U var in fix(_comp_compgen_*): avoid conflicts with "-v var" #1028

After scanning the codebase, I'm currently leaning to option 2, i.e., adding -U var in _comp_compgen as in the added commits of #1028.

@akinomyoga
Copy link
Collaborator

Hmm, #1026 is this one, and #1025 was merged an hour or so

Ah, sorry. That is a typo of #1025.

@scop
Copy link
Owner Author

scop commented Aug 13, 2023

SGTM. I have noticed #1028 and related changes across. It's however something that I'd like to understand better than what I can immediately pick up from a casual skimming review, that's why I haven't commented on that stuff yet. But with the limited understanding I have at the moment, the -U approach gives me a better feeling.

@scop scop merged commit 8a76f3d into master Aug 13, 2023
@scop scop deleted the feat/xrandr-setmonitor-3rd branch August 13, 2023 10:26
@akinomyoga
Copy link
Collaborator

Thank you. You may already notice it, but doc/api-and-naming.md of #1028 is the description for the option -U.

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