[Approved] Added proper horizontal scrollbar to filmstrip (1 Viewer)

[pilehave]

For some reason, a vertical scrollbar was attached to filmstrip. I guess someone never got around to finishing it, the code was really weird

I also decided to fix the horizontal scrollbar, which did not scale when the window did. So the scrollbar would be just as high and wide in 720x576 as in 1920x1080. Not good.

I have added the following new attributes to filmstrip facade:

scrollbarYOff will move the scrollbar a number of pixels up or down, calculated from the posY of the filmstrip-control
scrollbarWidth and scrollbarHeight are added, and can be used to set the size of the scrollbar (just as in coverflow)

The scrollbar now allows for the user to click and hold to navigate the filmstrip or to point and click to move a percentage left or right. It works like a normal scrollbar in any browser and same way as it does in listview etc.

While I was at it, I also fixed a render problem which made images in filmstrip disappear when the window was resized.

I have attached my patch and a screenshot of filmstrip.

EDIT New patch attached, I moved scrollbarfixes to this thread to keep things clean:
https://forum.team-mediaportal.com/...tical-horizontal-scrollbars-93059/#post710074

EDIT 2 Removed scrollbarXOff as I think it should just position in the centre of the filmstrip-control. Also removed the option to show/hide scrollbar, as this IMHO should be up to the settings in MediaPortal Configuration to decide.
New patch attached, old removed.


Example with a screenwide filmstrip


Example with a filmstrip positioned to the right. Scrollbar still centered

 

[tourettes]

It works like a normal scrollbar in any browser (stolen mostly from coverflow, thanks Andy!).

That makes it sound like there will be ugly copy & paste code which we don't want to have. Instead you should create a common scrollbar class that both views are utilizing (and maybe some other places as well).

 

[pilehave]

It works like a normal scrollbar in any browser (stolen mostly from coverflow, thanks Andy!).

That makes it sound like there will be ugly copy & paste code which we don't want to have. Instead you should create a common scrollbar class that both views are utilizing (and maybe some other places as well).

Please review the code before judging...

 

[tourettes]

It works like a normal scrollbar in any browser (stolen mostly from coverflow, thanks Andy!).

That makes it sound like there will be ugly copy & paste code which we don't want to have. Instead you should create a common scrollbar class that both views are utilizing (and maybe some other places as well).

Please review the code before judging...

Well if you already state that the code was copied mostly (or even partly) from a different class then it is clearly an indicator that there is a design issue.

 

[pilehave]

That makes it sound like there will be ugly copy & paste code which we don't want to have. Instead you should create a common scrollbar class that both views are utilizing (and maybe some other places as well).

Please review the code before judging...

Well if you already state that the code was copied mostly (or even partly) from a different class then it is clearly an indicator that there is a design issue.

Having a bad morning? If you don't even bother looking at the code, why are you even here?

 

[tourettes]

why are you even here?

Sometimes I wonder that myself as well...


About the patch - I dont have access to source codes currently so I have only your word that lot of code was copied. Shouldn't that be a sign / indicator of a bad design? Lately we have been cleaning a lot of copy & paste code in MP1.2.0b and we dont want to introduce new one. Patch / feqature itself might be a good one but the approach should be a different one - trying to reduce the copy & paste code when ever possible if new GUI components are added (or anything else). It will make the code much more easy to maintain when there is no copy & paste code around.

 

[pilehave]

why are you even here?

Sometimes I wonder that myself as well...


About the patch - I dont have access to source codes currently so I have only your word that lot of code was copied. Shouldn't that be a sign / indicator of a bad design? Lately we have been cleaning a lot of copy & paste code in MP1.2.0b and we dont want to introduce new one. Patch / feqature itself might be a good one but the approach should be a different one - trying to reduce the copy & paste code when ever possible if new GUI components are added (or anything else). It will make the code much more easy to maintain when there is no copy & paste code around.

OK, maybe you emphasize the word copy a bit too much, it was just a question of getting the scrollbar to calculate the percentage correctly. Andy already did these 5 lines or so in coverflow.

Before my patch, the code for implementing the horizontal scrollbar was this:

///TODO: add horz. scrollbar to filmstrip

Wow...quality coding, must say that!

Now it works.

The part I copied was this:

+                // We require mouse support for the scrollbar to respond properly.  Temporarily bypass the global setting to allow
+                // the action to work for us.
+                bool ms = GUIGraphicsContext.MouseSupport;
+                GUIGraphicsContext.MouseSupport = true;
+
+                _horizontalScrollbar.OnAction(action);
+                int index = (int)((_horizontalScrollbar.Percentage / 100.0f) * _listItems.Count);
+                _offset = index;
+                _cursorX = 0;
+                OnSelectionChanged();
+
+                GUIGraphicsContext.MouseSupport = ms;

I don't see how you would put that in a separate class for both filmstrip and coverflow to utilize, but if you feel that's the right way, then you are more than welcome to do so.

Alternatively, just remove the entire existing scrollbar part of MediaPortal, because it is full of bugs and utterly useless.

 

[tourettes]

OK, maybe you emphasize the word copy a bit too much, it was just a question of getting the scrollbar to calculate the percentage correctly. Andy already did these 5 lines or so in coverflow.

I think we had some communication issue. From the 1st post I got the feeling that majority of the code was copied.

Before my patch, the code for implementing the horizontal scrollbar was this:

///TODO: add horz. scrollbar to filmstrip

Wow...quality coding, must say that!

Having bad quality code in current revision is not an excuse to have new code with low quality (note that this is not related to your patch at all, but just in general that the current horrbile code quality level is some place cannot be used an excuse to add more klugde next to it ).

The part I copied was this:

+                // We require mouse support for the scrollbar to respond properly.  Temporarily bypass the global setting to allow
+                // the action to work for us.
+                bool ms = GUIGraphicsContext.MouseSupport;
+                GUIGraphicsContext.MouseSupport = true;
+
+                _horizontalScrollbar.OnAction(action);
+                int index = (int)((_horizontalScrollbar.Percentage / 100.0f) * _listItems.Count);
+                _offset = index;
+                _cursorX = 0;
+                OnSelectionChanged();
+
+                GUIGraphicsContext.MouseSupport = ms;

I don't see how you would put that in a separate class for both filmstrip and coverflow to utilize, but if you feel that's the right way, then you are more than welcome to do so.

You are right, there is no need for having a common code for such. Althou the MouseSupport seems to be a hack and should be better handled in the MP in general

 

[pilehave]

I have removed the mouse-bypass and I also fixed the vertical scrollbar. It makes sense to respect the users setting on mousecontrols.

For some reason, the vertical scrollbar's 100% was the number of items + 1, so the handle would never reach the bottom. Same problem in TVGuide, so I'll also provide a fix for that one as well. It also never scrolled past the number of channels when in singlechannel-view, so if you (like me) only have 8 channels, scrolling in singlechannel-view would not update the scrollbar beyond 8 lines...

I'll drop a patch sometime in this week, I just need to fix something else in TVGuide; currently, when you use "PgUp" when in singlechannel-view and focused on one of the time buttons, you can scroll into a deadend...

 

[pilehave]

Updated patch attached (see first post), I removed the mouse bypass so it is now controlled by the settings like in the rest of the skin.

I moved the fixes for scrollbars to this new patch:
https://forum.team-mediaportal.com/...tical-horizontal-scrollbars-93059/#post710074