-
-
Notifications
You must be signed in to change notification settings - Fork 32
feat: add option to escape characters in VarList.cpp #51
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs to be optional
Add a new ctor to avoid breaks
I don't know if this does break ABI |
Ok i just learned the lesson the hard way, i installed this pull request and restarted Hyprland and i won't launch ill make changes |
I don't know how to do this without ABI breaks i don't really know that much about c++ |
this shouldnt have abi breaks now. can you fix the conflicts? |
There you go. But i tried to launch Hyprland with this installed and it was unable to launch. |
Maybe you still have the bad so, I don't think adding a ctor breaks abi edit: launches fine on my end :) |
Ok maybe it was just me but any changes i did to the header file I didn't launch |
Ok now should be ready |
I just realized that i was crashing because i didn't change anything in aquamarine so it was reading the gpu list badly |
Ok, is that looking better? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah getting better
Sorry, i could not access my laptop yesterday |
CVarList list3("test:test\\:test", 0, ':', true); | ||
EXPECT(list3[0], "test"); | ||
EXPECT(list3[1], "test\\"); | ||
EXPECT(list3[2], "test"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test will fail, because it's bad... test string should have \\\\
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't failing on my end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well then it's wrong? You have two tests that are the same (look this and list2) and one of them expects one thing and the other expects a different thing LOL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but in the test list3 it is not using the new constructor parameter for handling character escaping so it is only splitting the string by the separator and keeping the \
. Or wasn't that the test you wanted me to add?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted you to add a test for escaping a backslash before a comma:
CVarList list3("test:test\\\\:test"...)
essentially we want two backslashes in the string before the colon, which should not escape the comma (because the backslash is escaped)
I've added option to escape a character on the VarList
Also added a test case for it
Closes hyprwm/aquamarine#167