Added room permanence option to server

[Assistant]

Added the --rooms-dir [directory] option to the server.
This allows the specifying of a directory to be used to read and write room data in order for playlists to persist permanently, regardless of every watcher leaving or the server restarting.
The behavior won't take place if the directory does not exist, or if the --isolate-rooms option is enabled.

[Et0h]

I've done a bit of testing and it seems like the position is not saved if people leave without pausing and when people join an empty room it doesn't correctly load the position either.

[Et0h]

I can see why it would be useful for those running private servers, but there might also be drawbacks which I have not thought of so I'll leave this open for comments to see what people think of this proposal before making any decisions as to whether to approve it in principle. It is possible that it might work best if this feature could be linked with a feature where a list of rooms which always show up in the room list can be specified (with the playlists being saved optionally being limited to this list).

[Assistant]

I understand the concern about drawbacks—I sat on this code for about a year because originally it modified the behavior directly, and I didn't want to submit it until I refactored it to be optional, which I didn't have the motivation to do until now. However, besides that initial oopsie with the roomsDirPath argument validation, I believe the only difference between running this fork without the flag and the original should be a few functions running that immediately return.

I also can't overstate how useful it is for those of us running private servers that watch serial content with friends. Going from using regular syncplay to syncplay with persistent rooms was a QoL improvement on the same level as going from counting 3, 2, 1, go on irc to using syncplay. Being able to plan and setup viewing sessions ahead of time, not having to remember where you left off, not having to re-setup everything every time, being able to see what your group has watched together, and probably other minor improvements I'm not remembering.

Anyways, regardless of the outcome of this PR I'll continue working on this feature.

[Assistant]

Also thanks a bunch for the feedback, it's greatly appreciated

[daniel-123]

I feel like this would be pretty useful functionality for me personally and likely for quite a few other users.

From technical standpoint I'd prefer to see the persistent storage as SQLite database rather than a flat file with json in it. My reasoning is as follows:

• SQLite is a very robust library that allows for abstracting away most filesystem related problems - both about security and reliability.
• We already use SQLite in the server for basic statistics (Qt SQL from pySide), so there is no dependency growth.

From perspective of UX I think the default behavior should be that unused rooms either fade away (for example if nobody enters them for a month or a year) or/and get deleted if their playlist is empty. With those two it could even be possible to enable them along with room isolation (though I'm not convinced if that's a good combination).

I'm completely on the fence about whether there should be two types of persistence - "full" i.e. a list of rooms that never get deleted and "semi persistent" which would disappear under some specific conditions. This would be a bit complicated to convey properly in UI and easily perceived as inconsistent/buggy. Though if we assume that whoever controls the server also is an expert who can manage that as a niche feature it might make sense?

All that said - switching over to Qt SQL would require a lot of changes to the PR, so I'd understand if you weren't up for it. I also don't consider it a strict requirement before accepting the PR but rather an idea to think about.

EDIT: Another small thought - another possibly useful variant of persistence would be short-term one, i.e. just for few minutes or something along those lines. That could easily default to "on" and would already provide some use as it would keep the room state across server restarts and network issues.

[Assistant]

I've made persistent rooms keep track of the time since last activity, and they get wiped if tried to interact with after a threshold (1 year by default, feel free to change this). It's configurable through --rooms-timer, and allows disabling with 0.
Rooms without playlists are already deleted when the last user leaves, as of 5885a6e.
I believe that should take care of the UX, although I'm not sure about isolated rooms, I don't use them so I don't understand them fully, so I didn't want to mess with them.

Regarding using a database, despite liking files from a sysadmin perspective as they're much easier to work with than a database, I do think a database would be best, however I'm not really up for it to be honest.

I think the idea in your edit is probably the best way to implement this feature, however I'm not confident enough to make a change like this to the default behavior of the program. If someone else wants to take a shot at it I think it'd be fantastic, though.

I also had the idea to make an option to disable creating new persistent rooms, only allowing rooms that already existed on boot to persist. What are your thoughts on that? Ideally I'd want to do something with the existing permissions system to allow operators to create new persistent rooms even in this limited state, but that's beyond my expertise and not something I'd use personally anyways.

[tntmod54321]

This seems like it would be an excellent feature! +1 for adding it

[Tremolo4]

I merged this feature into my private server and very much enjoy it, thanks @Assistant!

To add to the discussion: I think rooms should not be persistent by default and that there should be a way to explicitly set and unset them as persistent from the client, if the server allows it. That way one-time users don't accidentally leave stuff behind on the server.

As for the UI, I imagine being able to right-click on a room and having a persistence-toggle in a context menu. Alternatively, have another column in the room list named "Persistent" with a checkbox.

[Et0h]

My current thinking is as follows:

• Rather than calling it permanence, we can instead call it 'persistence' in line with Tremolo4's suggestion that it be something you can enable/disable for a given room.
• In the first instance we should implement a server-side implementation of room persistence as the work on that is pretty advanced. However, in the future we could allow for client-side persistence for servers that don't have this feature supported/enabled. This would in effect be the automatic playlist save/load feature suggested in Shared playlist quick save/auto save to file #309 and Shared playlist quick save/auto save to file #447.
• To allow for an ultimately consistent system between server and client, it makes sense that persistent rooms show up in the list of rooms even when you are not in them to make it easier to switch to those rooms and disable persistence in them.
• Because rooms can be manually deleted without much difficulty the timeout feature is not needed so it might want to be removed in the interests of simplicity and predictability. However, as a way of helping people to figure out whether or not a room has fallen out of use then we could make the last updated information accessible to users.
• Similarly, because the rooms would always show up while they were in persistent mode, it reduces the justification to have a list of permanent persistent rooms. In the interests of keeping simple it might be best not to add this feature, but at any rate it does not need to be in the initial implementation.

[Assistant]

As long as I can configure it to run on my server with my usecase, which is all rooms are persistent forever without having to do anything special for every new room, I don't mind the details.

Having empty persistent rooms shown in the client sounds great, it's actually something we wanted, but no one wanted to touch the UI. However, we have 123 rooms in my server, so the option to hide them or have them under a dropdown would be awesome. Being able to filter them would be even better.

[Et0h]

I'll give your use case some considerations @Assistant. I hadn't thought that people might want 123 permanent rooms at once.

When you are talking about having "all rooms persistent forever" do you mean you want permanent persistent rooms which cannot be deleted by the user?

If so, do you need these permanent persistent rooms to be supplemented by persistent rooms which can be created/deleted by any user or would you be happy for user-specifiable persistent rooms to be disable if you have configured Syncplay to use permanent persistent rooms?

In terms of UI, one approach to handling the high number of persistent might be to:

• Allow people to toggle which rooms are listed in the list of who is playing what (e.g. to allow users to enable or disable showing persistent rooms which don't have people in them).
• Always list persistent rooms in the room dropdown list next to the 'Join room button'. They could be listed after all of the user-specified 'remembered' rooms to give them priority.
• Add a remember/forget room option when you right click on a room in the list of rooms.

For reference, what would be the maximum number of rooms being actively used simultaneously that you would expect in your use case? For example, are there 123 rooms but only 20 are used at any one time or would most of them be in use at the same time?

[Assistant]

When you are talking about having "all rooms persistent forever" do you mean you want permanent persistent rooms which cannot be deleted by the user?

Sorta, a room could currently be deleted by having its playlist emptied I think. I guess explicitly not allowing users to delete rooms isn't something I need, since it hasn't come up, but giving them the ability to in a more direct, such as more obvious UX for it, way isn't what I want either.

If so, do you need these permanent persistent rooms to be supplemented by persistent rooms which can be created/deleted by any user or would you be happy for user-specifiable persistent rooms to be disable if you have configured Syncplay to use permanent persistent rooms?

I'd be happy with users not being able change the setting if it's set on the server, I'd prefer it actually.

For reference, what would be the maximum number of rooms being actively used simultaneously that you would expect in your use case? For example, are there 123 rooms but only 20 are used at any one time or would most of them be in use at the same time?

We only use a single room most of the time, 2 rooms at once isn't rare, and we've done 3 before, but the large number or rooms isn't due to a large number of users. We're just around 8 people that regularly watch stuff, with each other and other friends. We just have a room for each show we watch, or each kind of watching we're doing. We make funny names for each room, we use them to keep track of everything we've watched and where our progress is, and we've been doing it for a while, we've actually been sitting on this change for a while, but the code was terrible and definitely worthy of a PR.

In terms of UI, one approach to handling the high number of persistent might be to:

The first two sound great, and the third one does too if it defaults to not remembering.

[Et0h]

Okay I've pushed an initial implementation of persistent/permanent rooms using sqlite to https://github.com/Syncplay/syncplay/tree/room_persistence

So far this only changes things server-side, and it works as follows:

• If --rooms-db-file [path] is specified then server-side persistent rooms will be enabled. This will create/use the specified sqlite database file to store the playlist and other room state information. Persistent rooms will be deleted if there are no users or playlist in them. Syncplay will list all persistent rooms in the list of rooms.
• If --permanent-rooms-file [path] is specified then it will load a textfile from that path with one permanent room per line. Permanent rooms will always be listed and not delete even if it has no users of playlists in it.

The next step would be to provide a client-side way to toggle whether or not persistent rooms will be displayed (and to make them show up in the join comobox even when they are hidden).

[Et0h]

I've been thinking about how to handle support for non-persistent (temporary) rooms for servers where persistent rooms are enabled. The approach I've just implemented is so that if you put -temp in a room name then it will make it a temporary room and not save the data and then use the MOTD system to inform users of how they need to use this feature if they want their room to be temporary.

This approach has the benefit of not requiring client UI changes which in turn makes it backwards-compatible. For those on the latest version of Syncplay the MOTD notice is added by the client which makes it use the client's language, but for older versions the server sends it in the server's language. Given that room names and MOTD messages are long-standing feature this means that it will work all the way back to the earliest versions of Syncplay.

As part of this process I noticed that the server didn't seem to use the system language for messages, so I've tried to fix this. @albertosottile can check to see if this broke anything in macOS as I had to tweak that code to get things to work on Windows.

...I'm hoping that this feature is now pretty much complete and ready for testing.

[Et0h]

Okay, did a bit of testing the other day and it has a few bugs to fix before it is ready for testing. I'll post here when these are fixed.

[Et0h]

Okay, it should be actually ready to test now. Known issues:

• Currently no way for clients to filter out empty rooms.
• Things look fine in the GUI, but the userlist is a bit messy in CLI mode.

[Et0h]

Updates:

• New GUI doesn't show blank users for empty rooms.
• New console UI don't see empty persistent rooms or get phantom userlist updates.
• The GUI window menu now has a 'Hide empty persistent rooms' option.

Are there any changes needed before this is considered ready for merging with the main branch?

[Et0h]

Those running built versions of Syncplay can test the client/server at https://github.com/Syncplay/syncplay/releases/tag/v1.7.0-persistentrooms1

[Tremolo4]

Those running built versions of Syncplay can test the client/server at https://github.com/Syncplay/syncplay/releases/tag/v1.7.0-persistentrooms1

Is there a way to delete persistent rooms from the client? I can't seem to find one.

Also, even if I call my room something like newtest -temp in order to make it not persistent, it is still saved and restored when I reconnect to the server.

My -temp room does get deleted once I restart the server.

[Et0h]

@Tremolo4 This is how it is supposed to work when persistent rooms are enabled in the GIT HEAD version of Syncplay:

• Temporary rooms exist so long as they have people in them. This is the same as how rooms operate without the persistent room feature enabled.
• Persistent rooms stick around so long as they have files in the playlist. If you want the rooms to be deleted you just delete everything in the playlist and join a different room.
• Permanent rooms stick around even when they don't have files in the playlist or people in the room.

If this is not how it is operating for you then please be more specific about what you are doing, if you are using the GIT HEAD version of Syncplay, and what you mean by a room being 'saved'.

[Tremolo4]

Thanks for the explanation!

I'm using 566f90b as the server, and the release you posted as the client. I've also briefly tested it with client v1.6.9, same behavior.

Permanent rooms only exist if I supply a permanent-rooms-file to the server, right? I don't do that currently.

What I'm seeing right now is when I create any room at all, and then leave it again immediately, without adding anything to the playlist, it does not disappear from the room list. If I add -temp to the name, it also does not disappear, until I restart the server.

The -temp rooms do not get saved to the sqlite database, unlike the other rooms. 👍

So it seems that it does not recognize that the playlist is empty for me? Maybe I'm doing something wrong still.

Edit: If I do add something to the playlist of a -temp room, and then leave it, the room and playlist is saved (with no people in the room) until the server is restarted. -temp rooms should get deleted immediately when everybody leaves, even if the playlist is not empty, right?

[Et0h]

@Tremolo4 Okay thanks for bringing this to my attention. It seems like something may have been broken as part of the merge process. I'll look into it.

[Et0h]

@Tremolo4 I think I figured out what went wrong, which is that the room deletion code was erroneously condition on no db file being specified. Please let me know if 7083d8c fixes the bug.

[Tremolo4]

That seems to have fixed it. Works flawlessly now for me so far.

Thanks a lot for your work on this feature!