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

CLOUD: Remove the saves sync finished OSD message #1916

Merged
merged 1 commit into from Nov 15, 2019

Conversation

@bgK
Copy link
Member

bgK commented Nov 5, 2019

When using the cloud save sync features, It seems to me there are too many user interface elements, I feel a bit overwhelmed. There is a blinking overlay icon indicating network activity, OSD messages indicating success or failure, and a progress dialog when the save / load dialog is open.

With this PR, I propose removing the OSD message indicating the save sync finished successfully. For me, it breaks the experience when it appears while in-game after the engine autosaved. IMO, the overlay icon disappearing is a good enough indication that the saves synchronized.

@Tkachov

This comment has been minimized.

Copy link
Contributor

Tkachov commented Nov 5, 2019

I didn't really test the feature by completing the game yet (well, I wanted to do that on Android and there was a problem with certificates), but I thought that OSD message might be a little bit distracting.

I think instead of removing we should add some options for user to specify whether they want to see it or not. Like:
OSD: always show / show on failure / never show
icon: always show / never show

Maybe also add some icon for "failure" and show it for some time on failure. Or even turn that icon in actual interactable element. If you click a "working" icon, it shows some popup with current progress (for the "what takes you so long and when you'll be done?" use-case) and if it's "failed" icon, it shows the reason and maybe some hints on how that could be resolved, maybe "retry" button as well.

@bgK

This comment has been minimized.

Copy link
Member Author

bgK commented Nov 5, 2019

An option for disabling the icon would be nice as the current design perhaps lacks a bit in subtlety.

However, I fail to see a case where the message discussed here would be helpful. If it worked, then it worked. No need to tell me.

@lotharsm

This comment has been minimized.

Copy link
Member

lotharsm commented Nov 5, 2019

I agree that there should be at least an option to disable the "success" OSD message since we get a notification in case of an error anyway. Coincidentally, I got asked if there's a way to disable the message since it seems to be quite disruptive...

@criezy

This comment has been minimized.

Copy link
Member

criezy commented Nov 5, 2019

I don't really see a case where having the save sync message would really be useful, so I would tend to agree with @bgK that it can be removed entirely rather than adding an option. We don't really want options for every feature that can have one, as we would end up with too many options, as well as unnecessary code that adds to the maintenance burden. And to me it seems that this is a case where we don't really need an option.

Note that this is not a strong opinion. For me the Dropbox notification for every synchronised saved was more disruptive as it was removing the focus from the ScummVM window and causing the system cursor to be displayed in addition to the ScummVM one (until I turned those notifications off). In comparison the OSD message is not as disruptive :-P

@bgK

This comment has been minimized.

Copy link
Member Author

bgK commented Nov 10, 2019

I'm going to push the other commit in this pull request to master as it is not controversial.

Regarding the OSD message, do we need more time to reach a consensus?

It's not very good for immersion when it appears after an autosave. The
cloud icon is a good enough indication that cloud synchronization
happened.
@bgK bgK force-pushed the bgK:cloud-experience branch from 7117e07 to 43847f5 Nov 10, 2019
@lotharsm

This comment has been minimized.

Copy link
Member

lotharsm commented Nov 12, 2019

@bgK: No objections from my side, looking forward to see this merged as-is.

@bgK bgK merged commit edf82fe into scummvm:master Nov 15, 2019
2 checks passed
2 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bgK

This comment has been minimized.

Copy link
Member Author

bgK commented Nov 15, 2019

Thanks for comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.