X11 Resource Manager support #561
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
So far, I mostly got some test cases for the entry parser. Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Incorrect lines are supposed to be silently ignored. Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Instead of getting the Debug output of a Vec<u8>, I now get actual strings to look at. Also, instead of just the first failure, I get all of them. Much better to work with. Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
It is an error if this function does not consume the whole input, so do not bother with returning the remaining data. Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
The diff speaks for itself. A logic error / typo. Signed-off-by: Uli Schlachter <psychon@znc.in>
I have no clue yet how the matching should actually work. I just wrote some code to make tests pass. This already passes 39 of the tests in the test suite and has just 13 failures, most of them because I ignore the class so far. Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
There is a test with: - database: First.second: 1 - instance: First.third - class: first.second Here, the first component of the instance matches the database entry, but the second component of the class matches. Thus, we really have to do both matches in lock-step. Signed-off-by: Uli Schlachter <psychon@znc.in>
I do not know what I was thinking when I wrote this piece of code, but it makes no sense, thus it has to go. This also adds a test case which would fail before this commit, showing how wrong this code was. Signed-off-by: Uli Schlachter <psychon@znc.in>
This commit makes the matching machinery keep track on how it matched an entry. So far, this just makes the commit more complicated, but does not actually change anything. A follow-up commit will use this information to rank matches. Signed-off-by: Uli Schlachter <psychon@znc.in>
Yay, the test suite passes! Yet, I think this is not done yet. Signed-off-by: Uli Schlachter <psychon@znc.in>
Right now, the implementation of check_match ensures that all matches
have the length of the longest of instance / class (which are
technically required to have the same length, but this cannot be
expressed in the API). Thus, the assert_eq! that I am touching here can
never fail.
However, with the following patch, this would no longer hold. Thus, this
commit adds tests that would fail the assert.
xcb-util-xrm does not return a match for these new tests. Xlib returns 1
in both cases.
The patch (This simply makes the code handle premature ends of matches):
diff --git a/src/resource_manager/matcher.rs
b/src/resource_manager/matcher.rs index 00d3055..c30f72a 100644
--- a/src/resource_manager/matcher.rs
+++ b/src/resource_manager/matcher.rs
@@ -153,10 +153,14 @@ fn check_match(entry: &Entry, resource: &[String], class: &[String]) -> Vec<Vec<
// the current component by staying in the same state (index).
let mut states = vec![MatchState::default()];
// Go through the components and match them
+ let mut result = Vec::new();
for (resource, class) in zip_longest(resource, class) {
let mut next_states = Vec::new();
for state in states.into_iter() {
if state.index == entry.components.len() {
+ if resource.is_none() || class.is_none() {
+ result.push(state)
+ }
// We are at the end of the entry and thus cannot continue this match
continue;
}
@@ -188,7 +192,8 @@ fn check_match(entry: &Entry, resource: &[String], class: &[String]) -> Vec<Vec<
states = next_states;
}
// We have a match if we reached the end of the components
- states.into_iter().filter(|s| s.index == entry.components.len()).map(|s| s.history).collect()
+ result.extend(states.into_iter().filter(|s| s.index == entry.components.len()));
+ result.into_iter().map(|s| s.history).collect()
}
fn compare_matches(match1: &[MatchKind], match2: &[MatchKind]) -> Ordering {
Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
No idea if this is correct. I tried my best. Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
The old code was a big, ugly match that rustfmt wanted to make worse. I hope that this new approach is a lot more readable. Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
I'm all too familiar with that feeling. :-)
Of course, 100%!
I don't know Rust, sorry, but I will try to take a look anyway (don't hold up the PR for that though).
I'm fairly confident that the test cases in xcb-util-xrm cover just about everything, so if you pass all the same tests it should be fine. That said, as you probably noticed anyway, writing it I also found a bug in Xlib that no one seems to have noticed in a couple of decades, so... |
This clippy lint now also triggers in the resource_manager code, so just handle it globally together with the other such cases. Signed-off-by: Uli Schlachter <psychon@znc.in>
|
Just scanning through the code I think unfortunately it'll be difficult for me to say anything meaningful given my lack of Rust knowledge, sorry. But as I said, the tests should provide a high level of confidence. By the way, a mail from 2008 I found back when working on this that you may find humoring:
https://lists.freedesktop.org/archives/xcb/2008-March/003344.html edit: and just now noticing this mail was about awesome. Funny how that works out. |
|
@Airblader No problem. Thanks a lot for taking a look and sorry for my not-self-explanatory code ;-)
Heh, but from long before I even knew about AwesomeWM. And the file they mention does not exist on my system. Whatever, since that's about X11, I bet I do not want to know the details. ;-) |
psychon
added a commit
that referenced
this pull request
Nov 28, 2020
[0] triggers a false-positive that prevents progress. Since I have no good idea what to do about that, my bad idea is to just disable this part of CI: [0]: #561 [1]: rust-lang/rust#79498 Signed-off-by: Uli Schlachter <psychon@znc.in>
Signed-off-by: Uli Schlachter <psychon@znc.in>
eduardosm
reviewed
Nov 29, 2020
These functions are not needed for the tests at hand and thus will not be implemented. That is exactly the meaning of unimplemented!() and not todo!(). Signed-off-by: Uli Schlachter <psychon@znc.in>
eduardosm
approved these changes
Nov 29, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: #551
Hey @Airblader, thank you very much for writing xcb-util-xrm. Before writing this, I had basically no idea about Xrm (although at the time I thought differently) and all I know is from reading the source code. I tried looking at Xlib's source, but... yeah... no. I also pretty much stole all your test cases and came up with almost none of my own.
Are you okay with this or do you see a problem?
(And if you know Rust and have too much time at your hands (which I doubt): What do you think of this implementation?)
(I worked similarly when writing the cursor support in this library, but cursors make so much more sense than Xrm...)
Boy, this was more work than I expected. The result also kind of speaks for itself:
I bet that there are behavioural differences between this an Xlib / xcb-util-xrm. I also bet that no one will ever notice.
There are definitely internal differences. This code does not try to de-duplicate database entries. The matching algorithm is completely different and does not use recursion, but instead does something similar to the "usual algorithm" for checking if a nondeterministic finite automaton accepts a word. The unit tests claim that my algorithm works...
This PR contains the "raw" commits that I did in the last two weeks on this branch. I did not try to clean up the history, but I am not sure if this really matters.
Anyway, this is definitely enough to query
Xft.dpi, which is the use case for which I started this.This code saw exactly zero testing besides what is in this PR. YMMV.
Oh and: I left out some of the API that Xlib / xcb-util-xrm provides, but that does not seem to be used much. For example, this cannot serialise a database into a String, there is no support for combining two databases, there is no API for appending entries to a database, ....
Edit: Yay, CI rustfmt and local rustfmt disagree about how the code should look like. :-(