Skip to content
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

Saving of extended state to file on exit is not done in default config? #484

Open
3 tasks done
portnov opened this issue Dec 26, 2023 · 10 comments
Open
3 tasks done

Comments

@portnov
Copy link

portnov commented Dec 26, 2023

Problem Description

It seems that with current default config, persistent state is not saved to file on exit.

Steps to Reproduce

  1. Use any contrib module that uses ExtensibleState with extensionType = PersistentExtension.
  2. Exit xmonad (mod-shift-q)
  3. Start xmonad again
  4. observe: state was not saved

Configuration File

https://github.com/xmonad/xmonad/blob/master/src/XMonad/Config.hs; especially the part

-- quit, or restart
    , ((modMask .|. shiftMask, xK_q     ), io exitSuccess) -- %! Quit xmonad
    , ((modMask              , xK_q     ), spawn "if type xmonad; then xmonad --recompile && xmonad --restart; else xmessage xmonad not in \\$PATH: \"$PATH\"; fi") -- %! Restart xmonad

Questions

  1. For me the problem was solved by
 , ((modMask .|. shiftMask, xK_q     ), writeStateToFile >> io exitSuccess) 

Maybe this should be done by default?

  1. In older versions of xmonad, the default config used to be
 , ((modMask              , xK_q     ), broadcastMessage ReleaseResources >> restart "xmonad" True)

and restart by itself contains a call of writeStateToFile. I have an old config, which called restart, so that line remained, and for me mod-q retained the state, while mod-shift-q lost it.

Why the new default config does not call restart? Is restart deprecated now? should explicit call to writeStateToFile be added to default config?

  1. I was a bit surprised about the fact that the state file is deleted the moment it is read. Why? If xmonad exits unsuccessfully (power failure for example), it seems to me it's better to have stale state than have none. Or?...

Checklist

  • I've read CONTRIBUTING.md

  • I tested my configuration

    • With xmonad 0.17.1
    • With xmonad-contrib master
@slotThe
Copy link
Member

slotThe commented Dec 26, 2023

It seems unwise to me to save the state across "real" exits. If something is really supposed to be that persistent, it should probably not only live in extensible state.

  1. In older versions of xmonad, the default config used to be

    , ((modMask              , xK_q     ), broadcastMessage ReleaseResources >> restart "xmonad" True)
    

and restart by itself contains a call of writeStateToFile. I have an old config, which called restart, so that line remained, and for me mod-q retained the state, while mod-shift-q lost it.

Are you sure about this? It seems like the current line was changed from

, ((modMask .|. shiftMask, xK_q     ), io (exitWith ExitSuccess)) -- %! Quit xmonad

in 2022, but that one has its roots in 2007!

@portnov
Copy link
Author

portnov commented Dec 26, 2023

It seems unwise to me to save the state across "real" exits. If something is really supposed to be that persistent, it should probably not only live in extensible state.

Hm. ExtensibleState provides very nice interface for contrib developers to store things. Does it have some drawbacks because of which it is not recommended to store it "across real exits"?
Suppose we want to store layout state (for example, specific configuration of X.L.BinarySplitPartition), so that the user doesn't have to configure it each day from scratch. XS.put / XS.get seem to be the obvious choice. Otherwise, contrib developer would have to write support of some sort of config files (yaml or ini or whatever). Or use external library as a dependency.

Are you sure about this? It seems like the current line was changed from

I was talking about "restart" part. Mod-q used to call restart, and now it does spawn.

@liskin
Copy link
Member

liskin commented Dec 26, 2023

ExtensibleState provides very nice interface for contrib developers to store things. Does it have some drawbacks because of which it is not recommended to store it "across real exits"?

The only drawback I can think of right now is that there might be some contrib modules that don't garbage collect its state, so you may end up with stored data for windows that were closed a year ago. I've been trying to remedy this whenever I came across it, but I'm sure there are modules that still suffer from this. And then obviously if a data type stored in ExtensibleState gets renamed or deleted, there's no mechanism to garbage collect that either, so it'll persist in your state file for eternity.

Other than that, I think it's just a matter of "we've always done it this way". Some xmonad users prefer to configure things statically in their xmonad.hs (workspace names, managehooks, etc.), and thus naturally expect quitting xmonad to reset the state. Other users (me included) rearrange workspaces and layouts and so on dynamically and then spend considerable effort keeping the X session alive for months to not lose this state, or, perhaps, use writeStateToFile.

I don't quite think it's worth changing the default as long as both groups are able to configure xmonad to suit their needs.


Note that I do carry some extra patches and modules to facilitate my workflow:

Together, they move the state to /run/user/…, store it atomically, don't remove it on start, use separate filename for each X session and write it every 60 seconds. This way, my xmonad can recover from a crash, but since it's stored in /run, it doesn't restore the state after a shutdown, kernel panic or when the battery runs out. And I don't really mind, I consider it a feature that every couple months I have to start from a clean slate.

@TheMC47
Copy link
Member

TheMC47 commented Dec 26, 2023

Tagging along: changing the default does indeed seem unwise, for the reasons mentioned above. Maybe we can facilitate/upstream some of @liskin's patches though. @portnov, maybe you can give it a go?

@portnov
Copy link
Author

portnov commented Dec 26, 2023

@liskin provided some thoughts why changing defaults in current state of xmonad-contrib may be not very wise.
I have some experience in writing extensible software, and usually when I see such situations I say, well, if we can't change core behavior, let's give people a possibility for customization.
So, idea is as follows:

  1. liskin@cc6c9f5 seems not to be breaking any behavior and just making sure we won't have a broken state file. I'd say this commit should go straight into xmonad-core.
  2. liskin@86a5761 : this is more like, well, I want store file there or you want to store it in other place... Suggestion: add a field to XConfig: extensibleStatePath :: IO FilePath, with default value which works exactly like things are currently. Similarly, to remove or not to remove the file could be an option.
  3. X.H.WriteState could be in xmonad-contrib, because why not? :)
  4. Ideally, to preserve compatibility and introduce new functionality, it would be good to have a new constructor to StateExtension. The name is hard... Well, let's say ExtensionSurvivesExit. Such ExtensionClass states should be written to file at exit, while PersistentExtension works like it does now. Obviously in such case we should have two different files, like, let xmonad.state be for what it is now, and a new one, xmonad.persist (or maybe even xmonad.db ? ;)), for state which should survive exits. So, current contribs continue to work without any changes, but it will be possible to write new contribs, which store their state between sessions. Contrib authors will have to deal with "garbage collection" carefully in this case.

Any thoughts?

@portnov
Copy link
Author

portnov commented Dec 26, 2023

  1. Or, maybe it is possible to write support of such state (just with API similar to ExtensibleState) in xomnad-contrib, without changes to xmonad-core?

@geekosaur
Copy link
Contributor

I'd just like to note that when we moved to storing state in a file instead of passing it directly on the command line, we initially had a bug where the state file wasn't being unlinked so the state was persisting across sessions. This led to a lot of complaints.

@portnov
Copy link
Author

portnov commented Dec 26, 2023

Ok, so persisting state across sessions shouldn't be by default.

As far as I see, it's not hard to implement something like XMonad.Util.PersistentState (in xmonad-contrib), which would

  1. Read the file on startup
  2. Store data in memory, for example, in IORef (or MVar, or TVar, whatever)
  3. Provide API similar to ExtensibleState: put, get, gets
  4. Provide method similar to writeStateToFile
  5. Save data on xmonad restart or exit

For 1 and 5, it would provide functions which the user will have to explicitly call in startupHook and key bindings for exit/restart, respectively. Probably xmonad core could simplify this part a bit for user, but maybe it shouldn't bother.

@TheMC47
Copy link
Member

TheMC47 commented Dec 27, 2023

Here are my thoughts:

  • the change should be entirely transparent and opt-in, no user config should be altered
  • we need to provide some sort of utility to garbage collect/reset the state, so unused or changed constructors are removed.
  • we shouldn't crash if the state is invalid, and preferably try to ignore invalid constructors and such

After sleeping on this, I'm skeptical I'm a bit concerned about the details if the UX. How should we handle window identifiers? How can we even know that the data we're reading is indeed a window, without knowing the module?

@portnov
Copy link
Author

portnov commented Dec 27, 2023

Well, I don't think we can invent universal garbage collector here. Most what we can do is provide provide API for contrib writers. For example, removeEntriesOnObsoleteWindows :: ExtensionClass a => (a -> Window) -> X (). But probably contrib writer can do it himself.

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

No branches or pull requests

5 participants