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

Unify Skin controls #1848

Merged
merged 33 commits into from
Nov 24, 2018
Merged

Unify Skin controls #1848

merged 33 commits into from
Nov 24, 2018

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Oct 17, 2018

This keeps common skin settings when changing skins.
Continuation of #1642, fixes Bug 1740513

Are the following COs okay for everyone?
[Skin],show_4decks

[Skin],show_4effectunits
[Skin],show_superknobs (previous [Master],... is used in only one official mapping)

[Skin],show_spinnies
[Skin],show_coverart
[Skin],show_big_spinny_coverart

[Skin],show_8_hotcues
[Skin],show_starrating]
[Skin],show_rate_controls
[Skin],show_eq_knobs
[Skin],show_eq_kill_buttons
[Skin],show_xfader
[Skin],show_loop_beatjump_controls

[Skin],sampler_rows = number of sampler rows
[Skin],sampler_row_N = trigger CO for sampler rows WidgetStack
[Skin],sampler_row_current/..next/..prev = COs to cycle throught sampler rows WidgetStack
[Skin],sampler_row_N_expanded

@daschuer
Copy link
Member

This works for me. Thank you.

@uklotzde uklotzde added this to the 2.2.0 milestone Oct 22, 2018
@ronso0
Copy link
Member Author

ronso0 commented Oct 23, 2018

Woooo, grep and sed are my friends now.
Please test.

@Be-ing
Copy link
Contributor

Be-ing commented Oct 24, 2018

There is now a merge conflict in res/skins/LateNight/deck_row_5_transportLoopJump.xml.

@ronso0
Copy link
Member Author

ronso0 commented Oct 25, 2018

(rebased)

<attribute config_key="[Master],num_decks">4</attribute>
<attribute config_key="[Master],num_samplers">64</attribute>
<attribute config_key="[Master],num_samplers">16</attribute>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you change this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copy/paste mistake apparently. thx!

res/skins/Deere (64 Samplers)/skin.xml Show resolved Hide resolved
res/skins/Deere/skin.xml Show resolved Hide resolved
res/skins/LateNight/skin.xml Show resolved Hide resolved
res/skins/Tango (64 Samplers)/skin.xml Show resolved Hide resolved
res/skins/Tango/library.xml Show resolved Hide resolved
res/skins/Deere (64 Samplers)/skin.xml Show resolved Hide resolved
<attribute config_key="[Master],num_samplers">64</attribute>
<attribute persist="true" config_key="[Master],show_mixer">1</attribute>
<attribute persist="true" config_key="[Master],show_eqs">1</attribute>
<attribute config_key="[Master],num_samplers">16</attribute>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert back to 64

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep. thx!

@ronso0
Copy link
Member Author

ronso0 commented Oct 26, 2018

not finished, yet. I'll also take care of SplitSizes.

@daschuer
Copy link
Member

Is this really still a 2.2 PR?

@ronso0
Copy link
Member Author

ronso0 commented Oct 28, 2018

Is this really still a 2.2 PR?

I'd say so.
I think it makes sense to have this in a major 2.2 update where users rather expect/accept to re-configure some skin settings than in a point release.
At which branch would you like to target it?

@daschuer
Copy link
Member

2.2 vs 2.3 it is just because we are past beta.
I don't expect issues due to letting that slip into 2.2 though.

res/skins/Deere/skin.xml Show resolved Hide resolved
res/skins/Deere/skin.xml Outdated Show resolved Hide resolved
@Be-ing
Copy link
Contributor

Be-ing commented Oct 28, 2018

8 hotcues setting does not work in LateNight.

I'm ambivalent about whether to merge this to 2.2 or master...

@ronso0
Copy link
Member Author

ronso0 commented Oct 28, 2018

8 hotcues setting does not work in LateNight.

fixed.

@ronso0
Copy link
Member Author

ronso0 commented Oct 28, 2018

I'm ambivalent about whether to merge this to 2.2 or master...

I'm convinced users will appreciate it when checking out different skins, instead of having to go to each skin's settings menu to restore options. So why wait for 2.3?

@ronso0
Copy link
Member Author

ronso0 commented Nov 3, 2018

(rebased, conflicts fixed)

@daschuer
Copy link
Member

daschuer commented Nov 3, 2018

I have just tested it without any issues, thank you.
However, I vote for merging this to master.
We do not know how many custom skins and mappings are effected out there and a normal beta period would be fair.
What do others think?

@Be-ing
Copy link
Contributor

Be-ing commented Nov 3, 2018

I'm also wary to rush this for 2.2 considering the potential for regressions. Although we have already identified and fixed some of those, I am concerned there may be more subtle regressions lurking here.

There is a new conflict since #1840 was merged.

@ronso0
Copy link
Member Author

ronso0 commented Nov 3, 2018

... and a normal beta period would be fair.

You're right.

@Be-ing Be-ing changed the base branch from 2.2 to master November 3, 2018 20:16
@Be-ing Be-ing modified the milestones: 2.2.0, 2.3.0 Nov 3, 2018
@ronso0
Copy link
Member Author

ronso0 commented Nov 9, 2018

Some improvements for Deere:

  • use button templates in the skin settings menu
  • convert category labels (MiXER, EFFECTS, ...) to toggles like in LateNight (more compact menu)
  • add MIXER toggle to the tool bar
  • fixed sampler rows selection

check!
deere_add-mixer-toggle

@ronso0
Copy link
Member Author

ronso0 commented Nov 9, 2018

I'd say we should merge this now before more conflicts pop up.

res/skins/Deere/skinsettings_button.xml Outdated Show resolved Hide resolved
res/skins/Deere/skinsettings_category_button.xml Outdated Show resolved Hide resolved
@@ -113,11 +113,17 @@

#Spacer22 {
background-color: #222222;
}
} /*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete commented out code

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I'll uncomment it then. It's often useful having such snippets ready for debugging and tests.

@Be-ing Be-ing merged commit fea7960 into mixxxdj:master Nov 24, 2018
@Be-ing
Copy link
Contributor

Be-ing commented Nov 24, 2018

Thanks for your work and sorry to make you rebase so many times for this branch.

@ronso0
Copy link
Member Author

ronso0 commented Nov 24, 2018

never mind, had to iron out some mistakes anyway..

@ronso0 ronso0 deleted the unify-skin-controls branch November 29, 2018 18:31
@nikmartin
Copy link
Contributor

Just FYI Tango skin loop controls are not visible on master branch, assuming it's this PR.

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.

5 participants