Skip to content

Profile support and documentation updates#130

Open
035059 wants to merge 2 commits intoFreeSpacenav:masterfrom
035059:master
Open

Profile support and documentation updates#130
035059 wants to merge 2 commits intoFreeSpacenav:masterfrom
035059:master

Conversation

@035059
Copy link
Copy Markdown
Contributor

@035059 035059 commented Dec 7, 2025

Adds profile support, plus updates documentation for multi-key button bindings previously merged in #127.

@jtsiomb
Copy link
Copy Markdown
Contributor

jtsiomb commented Dec 7, 2025

Very nice, thanks! I'll try to find some time to review this fully in the next couple of days. I have one comment from a very superficial first look through the diff.

You chose to make profile overrides in the form <profile>.<option> = ..., which I like, this is perfectly fine. But this choice seems to lead you to a needlessly complicated parsing method, which duplicates some of the existing code. That's not a big deal, and truthfully the parser is due for some major overhaul at some point, but I think it could be done much simpler if we followed a sort-of "ini-like" structure:

... global options ...
[foo]
... profile foo specific overrides with the exact same syntax ...
[bar]
... profile bar specific overrides ... and so on

This would allow the parser to just have a bunch of struct cfg, instead of the single one we have now, and keep a pointer to the current cfg starting from the global options one. Then as soon as a [<name>] is encountered, just swap the pointer and the rest of the parser remains unchanged.

If we really like the dot notation and want to keep that, a similar approach, though slightly more complicated could be used. If the name contains a dot, switch the cfg pointer to the appropriate config block for whatever name is before the dot, move the "line" pointer after the dot, and leave the rest of the parsing untouched. Then the pointer to the global config should be restored at the top of the loop again.

For the override to actually happen, I didn't look hard enough at that part of your implementation yet. The first thing that crosses my mind is some sort of bitfield describing which options are valid, but you may have come up with something better, I'll know when I take a proper look.

Again I only took a very superficial look at the diff, so if you did something like what I'm describing and I just entirely failed to realize it, I apologize. Let me know what you think.

@jtsiomb
Copy link
Copy Markdown
Contributor

jtsiomb commented Dec 30, 2025

I had a second look at these changes, sorry it took a while to get around to it.

I like the implementation, but I think there's a conceptual "mistake" in this. Profiles can't be global, they have to be client-specific. When the user opens 3 different applications which use 6dof input, they can't all be handled by the profile of the last one to launch. Each application needs to have its own profile active.

So basically profiles need to go into the client structure, they can't be global.

@jtsiomb
Copy link
Copy Markdown
Contributor

jtsiomb commented Dec 30, 2025

I think I'll close this pull request, because the changes are going to have to be fundamental, not simple tweaks. If you find the time and motivation to take a second stab at it, feel free to open a new pull request. But also keep in mind my comments about simplifying the config parser as well.

@jtsiomb jtsiomb closed this Dec 30, 2025
@jtsiomb
Copy link
Copy Markdown
Contributor

jtsiomb commented Dec 30, 2025

hmmm I gave it some more thought, and I think I might still want to merge some version of this.
I went to look through the code to see how I would implement application profiles per-client, and I realized that we're conflating two different things here, or at least I was, and not all settings can be per-client to begin with.
Button mappings can't be per-client for instance. When a button is pressed we can't do multiple separate things for each client.

In any case being able to have overrides for each application that would ideally be activated automatically is one thing, and needs to be per-client. But the notion of being able to have multiple global settings and the ability to switch between them is also valuable.

I'm reopening the pull request to re-examine how close it is to the second use case. I think just dropping the attempt to switch profiles automatically should be sufficient...

@jtsiomb jtsiomb reopened this Dec 30, 2025
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