-
-
Notifications
You must be signed in to change notification settings - Fork 276
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
xmonad-contrib part of "Make extensibleState primarily keyed by TypeRep instead of type names" #600
base: master
Are you sure you want to change the base?
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
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.
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.
@@ -9,8 +9,8 @@ packages: | |||
extra-deps: | |||
- github: xmonad/X11 | |||
commit: master@{today} | |||
- github: xmonad/xmonad | |||
commit: master@{today} | |||
- github: liskin/xmonad |
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.
Don't forget to change this!
@@ -24,14 +29,18 @@ module XMonad.Util.ExtensibleState ( | |||
, gets | |||
, modified | |||
, modifiedM | |||
|
|||
#ifdef TESTING |
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.
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.
Description
We've been using the String we get out of
show . typeOf
as 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
extensibleState
map is now primarily keyed by the TypeRep themselves, with fallback to String for not yet deserialized data. XMonad.Core now exportsshowExtType
which 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.md
filen/a I updated the
XMonad.Doc.Extending
file (if appropriate)