[Rejected] Bugs in MyMusicPlayingNow and GUIMusicOverlay (1 Viewer)

[pilehave]

Hi

Attached you will find a patch for MyMusicPlayingNow, and a proposal to fix another bug below this text.

Reading up on controltypes, the Wiki states that:

The id of the control. The id will couple the skin file to the code, so if we later on want to check that a user pressed a button, the id will be required and must be unique. For controls that will never be referenced in the code it is safe to set it to "1"

Well, then MyMusicPlayingNow has a bug

Because if refers to a control that has ID 1 (and BTW doesn't exist in B3W in MyMusicPlayingNow.xml). This means that all unused controls need to have a different ID than 1, leaving only 0 as an option.

Either the Wiki is wrong, or the code is.

Look at the source of MyMusicPlayingNow.cs:

private enum ControlIDs
    {
      LBL_CAPTION = 1, [INDENT][INDENT][/INDENT][/INDENT]//<--THIS IS BAD BECAUSE LATER ON:
[SkinControl((int) ControlIDs.LBL_CAPTION)] protected GUILabelControl LblCaption = null; //THIS ID IS USED HERE

And now the source of GUIMusicOverlay.cs:

    [SkinControl(0)] protected GUIImage _videoRectangle = null; //<-- OH NO, AGAIN
    [SkinControl(1)] protected GUIImage _thumbImage = null; //<-- OH MY!

Going through the skin-files of B3W, I see a LOT of controls with ID 1, and close to none with ID 0.

My fix is simple regarding MyMusicPlayingNow, but someone needs to check the code for similar bugs.

The bug results in close-to-useless messages like this:

2009-10-15 16:44:28.714116 [ERROR][MPMain]: GUIWindow:OnWindowLoaded id:1
ex:Object of type 'MediaPortal.GUI.Library.GUIImage' cannot be converted to
type 'MediaPortal.GUI.Library.GUILabelControl'. at
System.RuntimeType.CheckValue(Object value, Binder binder, CultureInfo
culture, BindingFlags invokeAttr)
at System.Reflection.RtFieldInfo.InternalSetValue(Object obj, Object
value, BindingFlags invokeAttr, Binder binder, CultureInfo culture, Boolean
doVisibilityCheck, Boolean doCheckConsistency)
at System.Reflection.RtFieldInfo.InternalSetValue(Object obj, Object
value, BindingFlags invokeAttr, Binder binder, CultureInfo culture, Boolean
doVisibilityCheck)
at System.Reflection.RtFieldInfo.SetValue(Object obj, Object value,
BindingFlags invokeAttr, Binder binder, CultureInfo culture)
at MediaPortal.GUI.Library.GUIWindow.OnWindowLoaded()
MediaPortal.GUI.Music.GUIMusicPlayingNow

Right...the code tries to convert all my images to labels

Browsing the code, I am chocked to see that GUIMusicOverlay not only uses ID1, but also ID 0!!! This means that musicOverlay.xml only, and only! must contain one control with ID 0, and one control with ID 1.

Another source of error-messages found

 

[arion_p]

Reading up on controltypes, the Wiki states that:

The id of the control. The id will couple the skin file to the code, so if we later on want to check that a user pressed a button, the id will be required and must be unique. For controls that will never be referenced in the code it is safe to set it to "1"

Well, then MyMusicPlayingNow has a bug

Because if refers to a control that has ID 1 (and BTW doesn't exist in B3W in MyMusicPlayingNow.xml). This means that all unused controls need to have a different ID than 1, leaving only 0 as an option.

Actually if you go through MyMusicPlayingNow.xml you will see that all unused controls have ID 0, and no control has ID 1. As a result the code refers to some control that does not exist in the skin (it shouldn't do that), but then the variable is not referenced anywhere either, so it should probably be removed altogether.

Browsing the code, I am chocked to see that GUIMusicOverlay not only uses ID1, but also ID 0!!! This means that musicOverlay.xml only, and only! must contain one control with ID 0, and one control with ID 1.

This is not true. GUIMusicOverlay is a separate window, and control IDs used in it do not conflict with control IDs in other windows. In general, if a control ID is used by more than one control in a window, then no control using this ID can be referenced in code declaratively, you can still iterate all controls from code. So this definitely is not a bug, it is however a deviation from the guidelines.

 

[pilehave]

But can we at least agree on that no control, whatsoever, should be used in the code if they have ID 0 or 1? Even though GUIMusicOverlay doesn't conflict with anything (for now) it deviates from the rest of the code.

I spend 2 hours trying to figure out why i kept getting the "Object of type 'MediaPortal.GUI.Library.GUIImage' cannot be converted to type 'MediaPortal.GUI.Library.GUILabelControl'" error. This wouldn't have been the case if the ID's had been proper

When you browse through all the other windows that has some kind of XML file reading, all ID's start with at least 2, some with 10 or 20. I think this is the correct way of doing it since all skinners know that if the ID is not 01 or 1, then look out, changing that control could cause problems...

 

[arion_p]

But can we at least agree on that no control, whatsoever, should be used in the code if they have ID 0 or 1? Even though GUIMusicOverlay doesn't conflict with anything (for now) it deviates from the rest of the code.

Yes, I agree. Guidelines are made to be followed and there should be a very good reason for not doing so.