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

[guilib] oninfo for controls #8169

Merged
merged 4 commits into from Oct 19, 2015
Merged

Conversation

@mkortstiege
Copy link
Member

mkortstiege commented Oct 2, 2015

This adds "oninfo" for controls. /cc @BigNoid, @phil65, @ronie

P.S: Messed up the original branch and pr (#7453) for some reason. Thanks @xhaggi for pushing this to a separate branch!

@mkortstiege mkortstiege force-pushed the mkortstiege:guilib-oninfo-attribute branch from 1c29c07 to f2cd690 Oct 2, 2015
@phil65

This comment has been minimized.

Copy link
Member

phil65 commented Oct 2, 2015

Now you only need to kill toggle behaviour, then this is good to go. :P

@HitcherUK

This comment has been minimized.

Copy link
Contributor

HitcherUK commented Oct 2, 2015

Does this also fix the crash when using <oninfo>Info</oninfo>?

@ronie

This comment has been minimized.

Copy link
Member

ronie commented Oct 3, 2015

Does this also fix the crash when using Info

nope, still crashes.

other than that, it seems to be working ok.

@mkortstiege

This comment has been minimized.

Copy link
Member Author

mkortstiege commented Oct 4, 2015

Will have a look at it within the next days again.

@ronie

This comment has been minimized.

Copy link
Member

ronie commented Oct 4, 2015

looks like it breaks 'info' in the fullscreen video window in Confluence.

it still works for skins who use the DialogVideoInfo.xml, but Confluence uses the method to toggle this info in VideoFullScreen.xml

@mkortstiege mkortstiege force-pushed the mkortstiege:guilib-oninfo-attribute branch 2 times, most recently from 6fb866e to efd5ad3 Oct 16, 2015
@ronie

This comment has been minimized.

Copy link
Member

ronie commented Oct 16, 2015

could someone enlighten me what the problem with the info toggle is?
i know there was some discussion about it, but i probably didn't pay much attention to it.
i promise this time i will :-)

@phil65

This comment has been minimized.

Copy link
Member

phil65 commented Oct 16, 2015

if we decide to allow oninfo inside dialogs then this would lead to unpredictable behaviour for the user because, depending on which control is focused, either another dialog opens (--> Actor info for example) or the actual dialog closes. That is very weird in terms of UX and shouldnt be done. So either we disallow oninfo inside dialogs or we remove the toggle. I'm fine with both.
It's basically a very similar issue as with onback on edit controls, @Razzeee was confused by that some weeks ago too if I am not mistaken.

@ronie

This comment has been minimized.

Copy link
Member

ronie commented Oct 16, 2015

the problem with removing the info toggle is that we create another ux inconsistency,
namely that 'c' does toggle the context menu, 'm' does toggle the osd menu, 'o' does toggle the onscreen codec info.. and so on.
over the years we've put great effort in it to make sure each button will both open and close it's dialog.

@phil65

This comment has been minimized.

Copy link
Member

phil65 commented Oct 16, 2015

well, "great effort" equals to adjusting some few lines in keymaps ;) But I admit, you´re also right, I didnt really take that into account. Perhaps a sane solution would be to disallow < oninfo > inside infodialogs?

@HitcherUK

This comment has been minimized.

Copy link
Contributor

HitcherUK commented Oct 17, 2015

Or keep the same logic for the official skin but let others do what they want - that way the only complaints will be in the skins specific thread and not our own (or at least we can redirect them to the specific skin thread).

@mkortstiege

This comment has been minimized.

Copy link
Member Author

mkortstiege commented Oct 17, 2015

Or keep the same logic for the official skin but let others do what they want

I think this will be pretty weird for the average users. The more i think about it, the more i think we should keep the toggling. With the upcoming onclick override we don't need oninfo in the various info dialogs.

@BigNoid

This comment has been minimized.

Copy link
Member

BigNoid commented Oct 17, 2015

With the upcoming onclick override we don't need oninfo in the various info dialogs.

Yeah that wont work for the actor list for example, but keeping the toggle is indeed better in terms of consistency. @ronie has a valid point, everywhere else the button that opens the dialog can also close it, so we should keep that.

@phil65

This comment has been minimized.

Copy link
Member

phil65 commented Oct 17, 2015

Yeah, the onclick override could make oninfo in info dialogs superfluous to some extent.

imo we should still disallow silly stuff like makin oninfo starting a script when actor list has focus and closing the actual dialog when sth else is focused. Behaviour could be completely different across different skins which would become very weird in terms of UX. It would be much better to just rely on onback to close modal dialogs.

Anyways, before we cannot find an agreement, merge in whatever you want, then look at the fallout for next release and adjust if needed then.

mkortstiege and others added 4 commits Jul 6, 2015
@mkortstiege mkortstiege force-pushed the mkortstiege:guilib-oninfo-attribute branch from efd5ad3 to 83825cc Oct 18, 2015
@mkortstiege

This comment has been minimized.

Copy link
Member Author

mkortstiege commented Oct 18, 2015

Rebased and removed the toggle nuke commit for now.

@ronie

This comment has been minimized.

Copy link
Member

ronie commented Oct 18, 2015

tested, the issue with 'info' in the fullscreen video window in Confluence is fixed now.

@Razzeee

This comment has been minimized.

Copy link
Member

Razzeee commented Oct 18, 2015

jenkins build this please

@mkortstiege

This comment has been minimized.

Copy link
Member Author

mkortstiege commented Oct 19, 2015

Build error is unrelated. How to proceed with this one?

@ronie

This comment has been minimized.

Copy link
Member

ronie commented Oct 19, 2015

ok to merge as far as i'm concerned.

@BigNoid, @phil65, @HitcherUK your thoughts?

@BigNoid

This comment has been minimized.

Copy link
Member

BigNoid commented Oct 19, 2015

Good to go imo

@HitcherUK

This comment has been minimized.

Copy link
Contributor

HitcherUK commented Oct 19, 2015

Go for it.

mkortstiege added a commit that referenced this pull request Oct 19, 2015
@mkortstiege mkortstiege merged commit 4f0c059 into xbmc:master Oct 19, 2015
1 check failed
1 check failed
default Merged build finished.
Details
@hudokkow hudokkow added this to the Jarvis 16.0-alpha4 milestone Oct 20, 2015
@MilhouseVH

This comment has been minimized.

Copy link
Contributor

MilhouseVH commented Oct 21, 2015

Hi, was it intentional that this PR would ignore the default action when clicking OK/Enter on videos while navigating in Files?

If you set Settings > Video > File lists > Default select action to Show Information, then navigate to a video in Videos > Files:

  • With this PR: clicking OK on the video doesn't open the Info panel, instead it starts playback
  • Without this PR: clicking OK shows the Info panel (as configured)

Reported on the forum.

@ghost

This comment has been minimized.

@mkortstiege

This comment has been minimized.

Copy link
Member Author

mkortstiege commented Oct 22, 2015

#8273 should take care of it. Thanks again @anaconda.

mkortstiege added a commit that referenced this pull request Oct 23, 2015
[videos] fix default select action after #8169 (thanks anaconda)
@mkortstiege mkortstiege deleted the mkortstiege:guilib-oninfo-attribute branch Oct 26, 2015
ksooo added a commit to ksooo/xbmc that referenced this pull request Oct 29, 2015
stevegal added a commit to stevegal/xbmc that referenced this pull request Jan 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.