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 positioning #3451

Merged
merged 4 commits into from Nov 1, 2013
Merged

Conversation

jmarshallnz
Copy link
Contributor

This builds on @pieh's original work in #1208.

@kibje, @ronie mind taking a look and having a play?

One thing that likely will cause issues is labels. I think what will happen is that they'll interpret based on how they're aligned. i.e. it will be the right edge if they're right aligned. This applies to everything except list labels, which do things correctly.

I could perhaps put another commit on top to make label positioning consistent if you think that makes sense (IMO it does, but I hate breaking stuff for skinners...)

@ghost ghost assigned jmarshallnz Oct 18, 2013
@jmarshallnz
Copy link
Contributor Author

7601461 removes the hack of right aligned labels. Breaks skins (e.g. Confluence has right aligned labels like this in Media Info for movies)

@jmarshallnz
Copy link
Contributor Author

jenkins build this please

@jmarshallnz
Copy link
Contributor Author

XSLT to transform old skins to new:

https://gist.github.com/jmarshallnz/7065945

Have run this on Confluence - will push up another commit in a moment fixing it up

@ronie
Copy link
Member

ronie commented Oct 23, 2013

i've been using it for some days now and haven't spot any regressions.
didn't see any issues when i tested the xslt either.

what i can't figure out though is what the < centerx > and < centery > tags are for?

@jmarshallnz
Copy link
Contributor Author

<centerx> allows you to specify where the middle of a control should be. e.g.

<centerx>200</centerx>
<width>100</width>

would place the control from 150 to 250.

@ronie
Copy link
Member

ronie commented Oct 25, 2013

got it, thx!

@ronie
Copy link
Member

ronie commented Oct 27, 2013

xbmc crashes when using either the Touched or re-Touched skin

http://www.xbmclogs.com/show.php?id=76072&mode=raw

@jmarshallnz
Copy link
Contributor Author

Updated. Fixes crashes, as well as another (longstanding) bug I discovered for groups without <width> inside other groups with specified posx - the right edge wasn't calculated correctly.

I've also reverted the change to posx - i.e. it will behave as it did in Frodo to allow easier releases for skins that wish to publish to both versions.

Finally, I've added a search n replace commit to switch confluence to using <left> and <top>. Up to @ronie and @JezzX as to whether that's taken or dropped.

@jmarshallnz
Copy link
Contributor Author

@ronie any idea as to whether you want the search n replace commit 6b214e9 ?

Jonathan Marshall added 4 commits November 1, 2013 19:55
…x> or <posy> had incorrectly derived width or height.
…>, <centerx> and <centery>.

The <posx>, <posy>, <width> and <height> are derived.
Also allows for percentage sizing/positioning, e.g. <left>25%</left>
…ight> behave the same for all controls.

Note: <posx> will still behave as it did in Frodo:
 * On label controls outside of containers that are right aligned, it's the RIGHT edge.
 * In all other cases (including labels inside containers), it's the LEFT edge.
@jmarshallnz
Copy link
Contributor Author

jenkins build this please

@ronie
Copy link
Member

ronie commented Nov 1, 2013

@jmarshallnz i see no real benefit in merging the last two commits.
a mix and match between the old and new positioning methods might
make it a tad more difficult to maintain, but it's certainly not a showstopper.
so if you have your reasons to get it in, by all means, be my guest :)

@MartijnKaijser
Copy link
Member

Agree with ronie on this. This makes reviewing a mess. Like said the conversion script seems to work great and skins already need to make way more adjustments so this small extra bit should not be a a large burden.

@jmarshallnz
Copy link
Contributor Author

@ronie: the last two commits bring Confluence up to Gotham's positioning system (no posx/posy anymore at all). Without them it stays with Frodo's (only posx/posy).

I'll merge without them for now, and put up a PR with those two commits for you to consider.

@jmarshallnz
Copy link
Contributor Author

jenkins build this please

@MartijnKaijser
Copy link
Member

We meant keeping the old posx for skins.

@jmarshallnz
Copy link
Contributor Author

Why break skins when we don't have to? It allows easier maintenance of a Frodo and Gotham branch simultaneously for skinners, which several skinners have specifically asked for. Making things easier for those that contribute is the highest priority. I have no problem removing the support when Gotham is established (i.e. for H**) but in the meantime I'm not sure how this alters review practices at all?

jmarshallnz added a commit that referenced this pull request Nov 1, 2013
@jmarshallnz jmarshallnz merged commit a52edf6 into xbmc:master Nov 1, 2013
@jmarshallnz jmarshallnz deleted the control_positioning branch November 1, 2013 22:43
@da-anda
Copy link
Member

da-anda commented Nov 5, 2013

please drop the "r" positioning support form the new left/right/top/bottom/center tags, they make no sense and only complicate it even more. Rather make "right" align the right corner of the group from the right side (same with bottom from bottom). There is absolutely no need for the "r" toggle anymore for those new tags. posx and posy still need it ofc.
Also - beeing able to align a control with dynamic height e.g. to the bottom would be a nice addition (could be used for the balloon tips in Confluence to make their rendering more straight forward)

@jmarshallnz
Copy link
Contributor Author

I mostly agree, but it really doesn't complicate things code-wise to be honest, and only complicates the skin if they use it. All measures are done from the left is a simple rule to remember. I'm not sure that measuring from the right of the parent is necessarily intuitive? It'd be useful to post on the forum thread and gather opinion.

You can align the content of a control with dynamic height (assuming they exist) to the bottom edge already I believe. (Not sure if any controls with dynamic height actually exist though!)

@da-anda
Copy link
Member

da-anda commented Nov 6, 2013

yes, I know it's not big deal code wise, but we should force skinners to use those tags correctly. And to me (and kib) measuring form the right is intuitive in case I use 150. It eases positioning quite a lot because you don't have to care about the width + changing the width won't change the offset. And if you look at it from a pure visual POV it's also straight forward.

@ dynamic height
possible that the 00 in defaults.xml broke my test - just read about other issues with this in the forums.

@MartijnKaijser
Copy link
Member

several skins now have skin labels which have wrong positioning on the Y axes. See Touched skin system info. THe CPU and memory usage bars are positioned too low

@jmarshallnz
Copy link
Contributor Author

See #3668

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

4 participants