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

Control access cleanup #4739

Merged
merged 11 commits into from Jul 4, 2014
Merged

Conversation

jmarshallnz
Copy link
Contributor

This continues the work of #4731 cleaning up direct access to controls and replacing it with messages.

There's a couple of things to review:

  1. The nasty one is d743f93 which adds a method to specify spinner labels via a single message. Nasty due to the void* usage, though this is used elsewhere as well. Ideas welcome.
  2. There's a reasonably trivial change in the PVR guide stuff which I think can be clarified. @xhaggi, @opdenkamp, @Jalle19 please take a look at 0612a4a specifically, though a lot of this applies to the PVR dialog classes.
  3. I've introduced GUI_MSG_SET_FILENAME which is essentially identical to GUI_MSG_LABEL_SET. The only rationale I have is that by using a specific message we don't get the case of the message being sent to the wrong control type. I could reuse GUI_MSG_LABEL_SET for this though.
  4. The use of GUI_MSG_ITEM_SELECT for progress means that you can only set integers rather than floats. This may be a regression in behaviour, so perhaps an alternate is to attach a variant or some such to the GUIMessage Param?

Assuming 1 is acceped (or a better solution is suggested) I'll go through and redo other spots filling a spinner via the GUI_MSG_LABEL_RESET, GUI_MSG_LABEL_ADD, GUI_MSG_ITEM_SELECT technique which is inefficient as you're looking up the control for each message.

@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone May 21, 2014
@jmarshallnz
Copy link
Contributor Author

@Montellese mind taking a look at number 1 when you get the chance?

@Montellese
Copy link
Member

Looks fine by me as that's how it's used in other cases as well.
Would it make sense to add a SET_CONTROL_LABELS define to GUIMessage.h similar to the already existing ones like SET_CONTROL_LABEL etc?

@jmarshallnz
Copy link
Contributor Author

I could add a define (it would save a few lines in the few spots it's used atm).

@jmarshallnz
Copy link
Contributor Author

jenkins build this please

jmarshallnz added a commit that referenced this pull request Jul 4, 2014
@jmarshallnz jmarshallnz merged commit 47ccc2f into xbmc:master Jul 4, 2014
@jmarshallnz jmarshallnz deleted the control_access_cleanup branch July 4, 2014 22:32
@MartijnKaijser MartijnKaijser modified the milestones: Helix 14.0-alpha1, Pending for inclusion Jul 19, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants