xmonad-contrib part of "Make extensibleState primarily keyed by TypeRep instead of type names" - #600
Draft
liskin wants to merge 2 commits into
Draft
xmonad-contrib part of "Make extensibleState primarily keyed by TypeRep instead of type names"#600liskin wants to merge 2 commits into
liskin wants to merge 2 commits into
Conversation
…ep …" We've been using the String we get out of `show . typeOf` as key in `extensibleState`, but that has a somewhat serious bug: it shows unqualified type names, so if two modules use the same type name, their extensible states will be stored in one place and get overwritten all the time. To fix this, the `extensibleState` map is now primarily keyed by the TypeRep themselves, with fallback to String for not yet deserialized data. XMonad.Core now exports `showExtType` which serializes type names qualified, and this is used in `writeStateToFile`. A simpler fix would be to just change the serialization of type names in `XMonad.Util.ExtensibleState`, but I'm afraid that might slows things down: Most types used here will start with "XMonad.", and that's a lot of useless linked-list pointer jumping. Fixes: xmonad#94
geekosaur
approved these changes
Apr 25, 2022
geekosaur
left a comment
geekosaur
left a comment
Contributor
There was a problem hiding this comment.
TBH I don't see why you don't like the code here; it seems clean enough and in fact cleans up some existing code. In short, LGTM.
| commit: master@{today} | ||
| - github: xmonad/xmonad | ||
| commit: master@{today} | ||
| - github: liskin/xmonad |
Contributor
There was a problem hiding this comment.
Don't forget to change this!
| , modified | ||
| , modifiedM | ||
|
|
||
| #ifdef TESTING |
Contributor
There was a problem hiding this comment.
Does this really want to be conditional? Looks to me like you need it any time you're upgrading from an xmonad without this change.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
We've been using the String we get out of
show . typeOfas key inextensibleState, but that has a somewhat serious bug: it shows unqualified type names, so if two modules use the same type name, their extensible states will be stored in one place and get overwritten all the time.To fix this, the
extensibleStatemap is now primarily keyed by the TypeRep themselves, with fallback to String for not yet deserialized data. XMonad.Core now exportsshowExtTypewhich serializes type names qualified, and this is used inwriteStateToFile.A simpler fix would be to just change the serialization of type names in
XMonad.Util.ExtensibleState, but I'm afraid that might slows things down: Most types used here will start with "XMonad.", and that's a lot of useless linked-list pointer jumping.Fixes: #94
Related: xmonad/xmonad#326
I must admit I don't really like the code, especially the xmonad-contrib part. There's the assumption that a Right key maps to a Right value, and Left key to Left value, but enforcing that in the types probably means adding a another field to the XState.
Checklist
I've read CONTRIBUTING.md
I've considered how to best test these changes (property, unit,
manually, ...) and concluded:
(TODO)
n/a I updated the
CHANGES.mdfilen/a I updated the
XMonad.Doc.Extendingfile (if appropriate)