Removed the auto-refresh feature #1339
Conversation
This feature just generated a lot of unnecessary traffic to the server and hurt usability because the page would refresh unexpectedly while a user was actively looking at content. Fixes airsonic#1003, airsonic#1179, airsonic#729. Did not remove the column for that setting in the user database, since that would lead to complications if needing to downgrade for some reason.
|
Great, that's a good idea! From a UX perspective:
(Related to #1338) |
|
@fxthomas: I don't really want to change the design of the active top link (your first suggestion) in this PR since that is completely unrelated. And while I agree that your second suggestion of adding a refresh link in all the pages (like the random page already has) would be a good idea, doing so in a consistent place and fixing the table elements to have appropriate html spacing in all circumstances while also maintaining well formatted blocks in the jsp would require a pretty complete overhaul of the whole |
|
Okay, fair enough. Looks good to me then! |
| @@ -224,7 +224,7 @@ public void updateUserSettings(UserSettings settings) { | |||
| settings.getAvatarScheme().name(), settings.getSystemAvatarId(), settings.getChanged(), | |||
| settings.isShowArtistInfoEnabled(), settings.isAutoHidePlayQueue(), | |||
| settings.isViewAsList(), settings.getDefaultAlbumList().getId(), settings.isQueueFollowingSongs(), | |||
| settings.isShowSideBar(), settings.getListReloadDelay(), settings.isKeyboardShortcutsEnabled(), | |||
| settings.isShowSideBar(), 60 /* Unused listReloadDelay */, settings.isKeyboardShortcutsEnabled(), | |||
There was a problem hiding this comment.
Can we remove this column instead?
There was a problem hiding this comment.
As I noted in the original PR comment, I chose not to remove the column and just code around it so that it would not cause a problem if users downgraded or otherwise switched between versions. I think it would be appropriate to remove it when we are preparing for the next major release, but for now I think it's safer not to.
There was a problem hiding this comment.
Can you maybe add a todo for us? Maybe a github issue so we can track it?
|
This has two approving reviews, and I think I've addressed @muff1nman's concern, so I'm inclined to merge this. Anyone else care to do the honors? |
This adds a refresh link to all homePager.jsp browse methods in a consistent place, so that users can easily refresh the proper frame without causing a whole page refresh that would interrupt the web player. This is a follow-on to airsonic#1339 which removed the auto-refresh feature. A side effect of this cleanup was to change all spacing between sets of elements to a standard 2em to the right which made the layout less convoluted.
This feature just generated a lot of unnecessary traffic to the server and hurt usability because the page would refresh unexpectedly while a user was actively looking at content. Fixes #1003, #1179, #729.
Did not remove the column for that setting in the user database (just coded around it), since that would lead to complications if needing to downgrade for some reason.