stoli / Metropolis

Continuation of Metropolis skin for XBMC
Other
23 stars 21 forks source link

Media Overlay extended text overlaps widgets #342

Closed jingai closed 8 years ago

jingai commented 8 years ago

Extended text (current album, song) needs to be hidden if a widget is currently visible.

jingai commented 8 years ago

@MacGyverr, if you already have a visibility condition for this that works, just let me know where it is.

If you don't, it's no big deal -- I'll work on this one tomorrow.

jingai commented 8 years ago

@MacGyverr, the visibility condition you have in your 'personal' version works, except for on Home1 style because widgets are displayed by default there.

What we need is a general way to determine if a widget is visible or not, but for some reason I'm not coming up with anything that works :/

MacGyverr commented 8 years ago

My idea after seeing your addon image was to see if the 4 widget types (weather, pvr, playlists, and plugin-based) could be merged better into a proper singular widget control that could do both vertical or horizontal; and poster or fanart or icon.
I probably could make a fake content-list and fill it with the weather widget info, then it would work more like the rest. the same for Pvr widget.
Use one list, but 4 fake buttons to scroll through it/them (up/down) (left/right) Set the image sizes with conditional includes based on type/category variables. So that once done, we'd have one widget to keep track of, and all future content will be added through the include_variables.xml (like my plugin and playlist widgets do now.)

?

jingai commented 8 years ago

I assume you meant to comment on Issue #347 ?

At any rate, that's fine if you want to try to factor things out and make a common widget, provided it doesn't end up getting convoluted. It's honestly not that hard to keep track of them if we keep the ID scheme going (8001, 8002, 8003, etc). The problem for this particular issue is that I need a way to know if they're visible. Control(8001).IsVisible(), for instance, doesn't seem to work.

MacGyverr commented 8 years ago

This works fine, except I forgot to hide it if help text is on (and in settings). https://github.com/MacGyverr/skin.metropolis-personal/blob/master/720p/Includes_MediaOverlay.xml#L13

MacGyverr commented 8 years ago

I wrote it while drinking a gigantic Margareta, so you might want to check it. :)

jingai commented 8 years ago

I've got a pint in my hand now. Which of us is the better judge then? lol

edit: yeah, that's the one I was pointing out. It does work, except for Home1, where the widgets are always visible (not only when focused).

Also, it isn't necessary to hide it when the PlotBox is visible. The PlotBox automatically shrinks to make space for it.

jingai commented 8 years ago

Nevermind.. I got it.. was a stupid oversight lol

jingai commented 8 years ago

Well I thought I did. I think I'm going to make widgets visible by default for all Home styles. This will simplify things and fix the problem as well.

jingai commented 8 years ago

Huh.. it's actually better this way anyway.

So, if anyone asks, it's.. by design ;)

jingai commented 8 years ago

Done, and the visibility condition is actually legible now. Booyah.

MacGyverr commented 8 years ago

It was very confusing for those of us that use the horizontal menus, so I did the work to fix them. The commit I just made gives you the ability to always display them if you want, use vertical or horizontal widgets, and doesn't cause any issues with the media overalys.
https://github.com/MacGyverr/Metropolis/commit/dda75fc00269dfda4e7f5481f63b823b69f43c13

jingai commented 8 years ago

Now it just doesn't show at all if a shortcut has a widget, whether it's visible or not. This needs to be fixed.

MacGyverr commented 8 years ago

Oh, I wrote a bunch of stuff to let it all play nice together, then removed it, but didn't check the overlay after. I will fix it tomorrow if you don't get a chance.

jingai commented 8 years ago

It's back to the same old problem. I could never get Control.IsVisible(800[0-3]) to work. That was why I 'fixed' it to always show the widgets lol.

MacGyverr commented 8 years ago

Fixed (for sure) https://github.com/MacGyverr/Metropolis/commit/e382ccdf416497991d1e06319726767e379b7385

jingai commented 8 years ago

I think maybe I'm not communicating the problem clearly.

I want the extended text in the overlay to show if there is no widget on-screen. If one is configured, but not visible, the text should display.

Basically, utilize the available space.

MacGyverr commented 8 years ago

That's what the last commit does. In 2 and 3, if there is a sub menu or widget showing, it wont show (to avoid running into it). It shows all other times there. In 1 if there is a widget showing it won't show, but if there isn't one, it will. The sub menus are irrelevant there.

MacGyverr commented 8 years ago

screenshot000

screenshot001

screenshot002

screenshot003

screenshot004

jingai commented 8 years ago

It's not working here though.. if I turn on the option to hide the widgets by default in Home1, it doesn't show the overlay text even if the widget isn't visible.

Am I missing something you haven't committed or something?

MacGyverr commented 8 years ago

There is no hide option, you said you wanted them to always display in home 1 and only display when selected in 2 and 3. You also said to hide all sub menus from 2 and 3 until selected.

MacGyverr commented 8 years ago

Here;https://github.com/MacGyverr/Metropolis/commit/dda75fc00269dfda4e7f5481f63b823b69f43c13#commitcomment-13442975

You said" 1) "Always Show Widget" option should only apply to Home1 and be enabled by default, 2) Submenus should only be visible when focused in Home2/Home3, 3) Weather Widget visibility should match the others (hidden until focused, unless Always Show is active on Home1), 4) Break out the horizontal widget stuff. It doesn't really belong in this commit."

I can't enable it by default, I could only make a "Hide if checked", so I left it out. They are always showing in home 1.

jingai commented 8 years ago

Yes, I originally said Home1 should always show the widgets, but recanted that in the text you just copy/pasted. It's fine to be an option provided the default is to be shown.

I added the Hide option in this commit: https://github.com/stoli/Metropolis/commit/356cbcaabce75d5d4b7d1a32c61bb01cc1396aea

jingai commented 8 years ago

Regarding not being able to enable it by default, what you do is just re-word the phrase to be negative. "Hide widgets when unfocused," for instance, which is what I did in https://github.com/stoli/Metropolis/commit/356cbcaabce75d5d4b7d1a32c61bb01cc1396aea

jingai commented 8 years ago

Bah.. didn't mean to hit close..

MacGyverr commented 8 years ago

Git is most definitely broken. untitled

Here is the line 43 that works off of what you wrote:

[[Skin.HasSetting(Home2) | Skin.HasSetting(Home3)] + !Control.IsVisible(9001)] + ![Control.IsVisible(9901) | Control.IsVisible(9902)] + ![Control.HasFocus(8000) | Control.HasFocus(8001) | Control.HasFocus(8002) | Control.HasFocus(8003)] | [![Skin.HasSetting(Home2) | Skin.HasSetting(Home3)] + [[Skin.HasSetting(HideUnfocusedWidgets)+ ![Control.HasFocus(8000) | Control.HasFocus(8001) | Control.HasFocus(8002) | Control.HasFocus(8003)]] | IsEmpty(Container(9000).ListItem.Property(widget))]]

MacGyverr commented 8 years ago

Had to delete and re branch (for the 4th time). https://github.com/MacGyverr/Metropolis/commit/0ba2d48f8402b2560dacc45d17880ba103fca2a6

Here is home1 with mediaoverlays because the widget is hidden due to skin.settings.

screenshot005

jingai commented 8 years ago

Any time upstream squashes commits from downstream, it has diverged. It will not auto-merge. Re-branching is what you're supposed to do. Each branch is supposed to be its own unique feature. When it's merged into upstream, it's expected that the branch that was merged will then be deleted (or at least go unused), unless the commit was merged without change, which would mean you could simply pull it in.

I have a personal policy that each commit I push into upstream will work on its own and provide a single feature/fix. Small, complete commits that only have one goal are very helpful when one needs to bisect to find the source of an issue.

I'm sorry if this seems like a PITA.. I can just cherry-pick and squash as I go if you don't feel like dealing with it. It's just how collaborative development with git usually works.

I'll test the new visibility condition you posted and, if it works in all spots, I will merge refactor into master.

jingai commented 8 years ago

Side-note.. you know how to delete a branch from your fork, right? You don't need to keep making new branch names. You can just do:

% git push origin :new-refactor
% git checkout master
% git pull
% git checkout -b new-refactor upstream/refactor

The first line there deletes the remote branch. It's kind of weird syntax, but basically with nothing on the left-side of the colon, it's telling git to "push nothing into the origin remote branch new-refactor".

jingai commented 8 years ago

Everything tests out fine now. Thanks @MacGyverr !