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

[3D] usability fixes and cleanup #5236

Merged
merged 9 commits into from Sep 23, 2014
Merged

[3D] usability fixes and cleanup #5236

merged 9 commits into from Sep 23, 2014

Conversation

@da-anda
Copy link
Member

da-anda commented Aug 17, 2014

This PR fixes several usability issues with 3D playback handling:

  • moves the "2D" stereoscopic mode from "Settings->System->Video->Preferred stereo mode" to "Settings->Video->Playback->Preferred stereoscopic playback mode" because 2D is no 3D mode.
    This fixes the issue that you couldn't use the 2D->3D toggle action/buttion if you preferred playback to be started in 2D and wanted to switch later to your preferred 3D mode, because preferred 3D mode had to be set to 2D in order to get 2D playback by default
  • makes "autodetect / same as movie" the default "preferred stereomode" and removes the "none" option from the list of preferred modes (a preferred mode has to be a valid mode and not NONE)
  • removes the select option of the movies 3D mode in the ask dialog in case the preferred mode was set to auto (which also means it's using the movies mode - thus option was superfluos in those cases)
  • changes the label of the "preferred stereo mode" option in the ask dialog to "same as movie" in case that's the preferred mode. Before we resolved the resulting preferred mode and show "side by side" or "top bottom", but this is IMO misleading as users confirmed
  • removes moves stereo mode "none" from preferred modes and adds a "ignore" playback mode instead
  • removes duplicate labels
  • improves usability of some labels

@elupus please review
@jmarshallnz - would also apply to Gotham if you consider these as relevant

@da-anda

This comment has been minimized.

Copy link
Member Author

da-anda commented Aug 19, 2014

RMs, is this something you want fixed for Helix? If so, we should get it in asap so that we get more user feedback than with the test build I linked in the forum. Needs a decision about the missing "none" case though - can readd it.

@Montellese I unfortunately forgot to test myself, so I'm just asking. I removed two settings options from a spinner - will the settings system detect that the probably user selected value is no longer valid and update/change to the default? I'm not sure if this is done automatically or if I have to provide a migration script (did some research in code but haven't been lucky)

@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Aug 19, 2014

imo this should be ok as usability improvement for Helix, given that code is approved. We shouldn't mess to much with Gotham any more.

@Montellese

This comment has been minimized.

Copy link
Member

Montellese commented Aug 19, 2014

@da-anda: It depends on whether the list of possible values is defined statically (i.e. in the setting's XML definition) or dynamically (i.e. through a C++ SettingOptionsFiller callback implementation). In the static case the settings library realizes that the value is invalid and falls back to the default value. In the dynamic case it doesn't detect the invalid value and considers it valid. In that case you would have to define an update path.

@da-anda

This comment has been minimized.

Copy link
Member Author

da-anda commented Aug 19, 2014

@Montellese thanks for info. Options are filled by a settings provider - so non-static. Can you point me to an example of an existing update path or just the location on where to register a custom update handler? As mentioned earlier - I browsed the settings C++ sources already but haven't spotted the correct place. Thanks.

@Montellese

This comment has been minimized.

Copy link
Member

Montellese commented Aug 19, 2014

@da-anda: Take a look at the screensaver.mode setting. It has <updates> defined in the setting's XML definition and as it is associated with CApplication (see xbmc/settings/Settings.cpp around line 715) it is handled in CApplication::OnSettingUpdate().

@topfs2

This comment has been minimized.

Copy link
Member

topfs2 commented Aug 19, 2014

Very quick review suggests that the patches seems to be quiet unlikely to break things. And useability fixes is always awesome. So given that someone knowledgeable about the code doesn't halt it I say merge.

The only thing I find curious is the almost arbitrary default=100 but I suspect that's outside the scope of the PR.

@da-anda

This comment has been minimized.

Copy link
Member Author

da-anda commented Aug 19, 2014

the default 100 is related to a special ENUM we use (RENDER_STEREOMODE_AUTO = 100). It has this high offset by intention. It's no real stereoscopic mode but some internal special case - and we choose a high offset to not get in conflict once we have to add more real 3D modes to the list of supported ones

@da-anda da-anda force-pushed the da-anda:3D-ux-fixes branch 3 times, most recently from ef687ef to 75312d0 Aug 20, 2014
@da-anda

This comment has been minimized.

Copy link
Member Author

da-anda commented Aug 20, 2014

updated PR.

Readded the stereomode "none" usecase so that users that don't want their 3D movies to be processed by our renderengine (some minimal conversions apply IIRC) have the freedom to choose so. I added it as "playback mode" option with label "ignore" though and not as selectable stereomode like before.

I also improved some labels based on user comments - should improve usability -> Nobody knew what stereoscopic is, so I appended "3D" everywhere. Only writing "3D" would IMO be wrong and could be confused with 3D engine (like in computer games) - videos are stereoscopic, even if marketing departments hype that feature with just "3D".

I also added a migration path for the changed settings and made use of ENUMs instead of integers for the playback mode options.

jenkins build this please

// if preferred playback mode was OFF, update playback mode to ignore
if (playbackMode == STEREOSCOPIC_PLAYBACK_MODE_PREFERRED)
CSettings::Get().SetInt("videoplayer.stereoscopicplaybackmode", STEREOSCOPIC_PLAYBACK_MODE_IGNORE);
stereomodeSetting->SetValue(RENDER_STEREO_MODE_AUTO);

This comment has been minimized.

Copy link
@Montellese

Montellese Aug 20, 2014

Member

You need to return the return value of SetValue() so that the settings system knows whether the update worked or not.

// if preferred playback mode was MONO, update playback mode
if (playbackMode == STEREOSCOPIC_PLAYBACK_MODE_PREFERRED)
CSettings::Get().SetInt("videoplayer.stereoscopicplaybackmode", STEREOSCOPIC_PLAYBACK_MODE_MONO);
stereomodeSetting->SetValue(RENDER_STEREO_MODE_AUTO);

This comment has been minimized.

Copy link
@Montellese

Montellese Aug 20, 2014

Member

Same as above.

@da-anda da-anda force-pushed the da-anda:3D-ux-fixes branch from 75312d0 to fb7aba6 Aug 20, 2014
@da-anda

This comment has been minimized.

Copy link
Member Author

da-anda commented Aug 20, 2014

@Montellese thanks for the pointers - I had seen the return values but had forgotten about them (was already late). The new/changed settings still applied though (just in case this should not have happened)

@da-anda

This comment has been minimized.

Copy link
Member Author

da-anda commented Aug 20, 2014

forgot to mention that PR is updated (sorry for extra spam)

@da-anda

This comment has been minimized.

Copy link
Member Author

da-anda commented Aug 26, 2014

@topfs2 is this good to go for next merge window? Not sure how I should interpret your comment. The only other knowledeable person for this code would be @elupus - but I fear he won't have the time to review. Anybody else is ofc welcome to review as well.

@da-anda da-anda force-pushed the da-anda:3D-ux-fixes branch from fb7aba6 to 6025edf Aug 26, 2014
@da-anda

This comment has been minimized.

Copy link
Member Author

da-anda commented Sep 1, 2014

jenkins build this please

@da-anda da-anda force-pushed the da-anda:3D-ux-fixes branch from 6025edf to e9ec754 Sep 2, 2014
@da-anda

This comment has been minimized.

Copy link
Member Author

da-anda commented Sep 2, 2014

rebased because jenkins was not happy with compiling. jenkins build this please

@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Sep 14, 2014

@topfs2 @jmarshallnz
can we get this in Helix?

@da-anda da-anda force-pushed the da-anda:3D-ux-fixes branch 2 times, most recently from dd9eade to e99ec4c Sep 15, 2014
@da-anda

This comment has been minimized.

Copy link
Member Author

da-anda commented Sep 15, 2014

jenkins build this please

da-anda added 3 commits Aug 15, 2014
…ereo mode (it's no real stereomode)

Also removes the mode "none" from the list of preferred modes, and moves it to the playback modes.
@da-anda da-anda force-pushed the da-anda:3D-ux-fixes branch from e99ec4c to 7002fb7 Sep 19, 2014
@da-anda

This comment has been minimized.

Copy link
Member Author

da-anda commented Sep 19, 2014

added some code cleanups. jenkins build this please

@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Sep 20, 2014

@topfs2 topfs2 added the Approved label Sep 23, 2014
@da-anda da-anda changed the title [3D] usability fixes [3D] usability fixes and cleanup Sep 23, 2014
da-anda pushed a commit that referenced this pull request Sep 23, 2014
[3D] usability fixes and cleanup
@da-anda da-anda merged commit 4503ad5 into xbmc:master Sep 23, 2014
1 check passed
1 check passed
default Merged build finished.
Details
@da-anda da-anda deleted the da-anda:3D-ux-fixes branch Sep 23, 2014
@MartijnKaijser MartijnKaijser added this to the Helix 14.0-alpha4 milestone Sep 23, 2014
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.