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

Deere 2.1 part 2 #1343

Merged
merged 54 commits into from
Dec 1, 2017
Merged

Deere 2.1 part 2 #1343

merged 54 commits into from
Dec 1, 2017

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Sep 16, 2017

Continuing work on the Deere skin. Now that I have been using it for a while, I am focusing on simplifying it in this PR.

I will be refactoring it to remove the bottom splitter that has been confusing users. Having two splitters in the same direction makes resizing them wonky at times. The only purpose of that was to make it possible to hide the EQ knob labels and have the mixer take less vertical space. Instead of making that optional, I'll just remove the EQ knob labels. LateNight and Tango already do not have the EQ knobs labeled. This may be a little bit confusing for new users completely unfamiliar with DJ equipment, but so are many other parts of the GUI and tooltips are good enough for those. To anyone who has ever used any DJ gear, it shouldn't be an issue at all.

TODO:

  • Remove bottom splitter
  • Simplify samplers
  • Add booth gain knob
  • Show recording & broadcasting icons when not active
  • Polish mixer and deck sizes in all configurations
  • Left click to unfocus effects
  • Find a better place for vinyl control buttons?
  • Find a better place for beatgrid editing buttons?

@daschuer
Copy link
Member

Good Idea!

</Template>

<Template src="skin:knob_toolbar.xml">
<SetVariable name="TooltipId">master_gain</SetVariable>
Copy link
Member

Choose a reason for hiding this comment

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

Wrong tooltip

<SetVariable name="group">[Master]</SetVariable>
<SetVariable name="control">booth_gain</SetVariable>
<SetVariable name="color">red</SetVariable>
<SetVariable name="label">Booth</SetVariable>
Copy link
Member

Choose a reason for hiding this comment

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

Now we have Gain / Head Gain / Booth .. three naming schemas for similar features.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm... haven't checked it at all, but here is my opinion:
"Gain", without any other label, should be used where the context limits it to a specific control. I.e. track gain, sampler gain, Master gain should tell that it is the Master.
"Head gain": if not using a hardware control (present in many controllers) the UI specific control for headphones should be labeled this way.
"Booth gain": AFAIK, the booth is the speakers/monitors that the DJ can have inside the cabin to listen to the audio. They are of importance when the cabin's position can't give the DJ enough detail. It can serve two purposes: to better equalize, or to better mix (getting a cleaner source of what's being played while also listening to the headphones). Some other times it might simply be for DJ amusement :P

Anyway, booth gain does not have much sense on Mixxx since we don't have an explicit Booth output and it should not be confused with headphones output. Booth is generally connected to the secondary output of a Mixer table, containing also the master output.

Copy link
Contributor Author

@Be-ing Be-ing Sep 16, 2017

Choose a reason for hiding this comment

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

I agree that "GAIN" without any context is not clear. I have changed it to "MASTER".

The booth output was introduced in #1279. The gain knob for it is only shown if a booth output is configured. It is useful so a sound card with sufficient output channels can be plugged directly into the main and booth speakers without the signal getting degraded by going through an unnecessary mixer.

Users were getting confused by the bottom splitter and having two
spltiters in the same direction made resizing them wonky. The only
purpose of the bottom splitter was to hide the EQ knob labels, so just
remove the EQ knob labels always.
There's some weird asymmetry going on in the mixer. Not sure why...
@daschuer
Copy link
Member

some time ago, we decided for the word "gain" to state clear that this is a digital pre-amplifier gain with a bitcrusher nature, useful to level the output for maximum signal noise ratio.
It is not a volume knob controlling a digital hardware pot, which is the expectation of most users.
As a general advise we can say "Don't touch"

If we drop the word gain, the issue is back. Can you add a Group label "Gain" for them?

Maybe it is time to work on this bug, to finally fix this annoying and long standing issue:
https://bugs.launchpad.net/mixxx/+bug/1440443
https://bugs.launchpad.net/mixxx/+bug/1440675
https://bugs.launchpad.net/mixxx/+bug/1458677

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 16, 2017

If we drop the word gain, the issue is back.

Whether it's labeled "MASTER" or "GAIN" in the skin is irrelevant to that issue IMO. Neither makes it clear that the knob does not affect the hardware volume control, nor do I think there is any succinct label that could make that clear. I understand your desire to try to implement proper volume control in an easy way that users wouldn't be likely to mess up, I do not think it is possible with the heterogeneity of operating systems and sound cards out there. Many sound cards do not even expose the hardware volume control to the computer. Even if they did, I think it would be futile to try to manipulate the hardware volume control reliably across each OS and all the complications of the various sound APIs on Windows. IMO the best we can do is document what the software gains do and advise not to use them in most cases.

Maybe it is time to work on this bug, to finally fix this annoying and long standing issue

Can we please focus on finishing what we have started and getting a release out?

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 16, 2017

The refactoring to get rid of the bottom splitter is done, but there is now some strange asymmetry between the left and right sides of the mixer I am trying to resolve. It is most apparent by right clicking the crossfader to center it and seeing that it does not align with the master level meter.

@daschuer
Copy link
Member

We had a condense about the GAIN label. An it is an issue for me that the controls are looking like volume in Tango.

@daschuer
Copy link
Member

Can we please focus on finishing what we have started and getting a release out?

We can IMHO release at any time. If we wait for all started things to be finished (like we did now for more than year) we will get never a release. The today master is IMHO stable enough to be a beta.
My vote was to release the last years September version now we have September again.

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 17, 2017

I am thinking about removing the "Minimal Controls" mode. I implemented that as a quick hack around the clutter of the old looping UI. Now that all controls fit into a reasonable area, I think it can be removed. I have found myself frustrated and confused a number of times when I had the Minimal Controls mode active and wanted access to the hidden controls. Thoughts?

@daschuer
Copy link
Member

I am thinking about removing the "Minimal Controls" mode.

That would be Ok for me. We can advertise Deere as a full feature skin. Tango and Schade can be focused on other use cases. An additional Broadcasting skin would be real great

Microphones can't be assigned to the crossfader in the engine.
This was a quick hack around the clutter of the old looping UI. Now that
all controls fit into a reasonable amount of space and the bottom
splitter has been removed, get rid of this option. On many occasions I
found myself confused why I couldn't access a control in minimal
controls mode then annoyed that I had to open the skin settings menu to
get to what I wanted.
TODO: Make a new knob widget to show the proper tooltip
@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 17, 2017

I added an option to hide the EQ kill switches. Few controllers have buttons for those so they waste space for most users. However, they're useful for mouse & keyboard users.

@ronso0
Copy link
Member

ronso0 commented Nov 13, 2017

I experience that the spacing via qproperty-layoutSpacing is not working reliably in Deere, instead several button grids look like this again, although we already fixed it:
grid_1
When I restart mixxx multiple times while testing the padding in between the buttons sometimes even vanishes completely. I had headaches suspecting I messed up my git branches. Git is fine but the problem persist.
Can anyone confirm this?

@ronso0
Copy link
Member

ronso0 commented Nov 14, 2017

Like @sblaisot I miss sampler gain. A knob in the top bar might be sufficicent, visible only when samplers are enabled.

@Be-ing
Copy link
Contributor Author

Be-ing commented Nov 14, 2017

I'm not pushing any more commits to this PR until I get write access. This is ridiculous that this branch has been sitting ready to merge for 1.5 months and pull requests are being targeted at my fork instead of master. The discussion has strayed far off topic from the original purpose of this PR.

@daschuer
Copy link
Member

Sorry for not following all details of this branch an merge it in a time. @ronso0: Is it merge-able in the current state? Do we have a finished review with a formal LGTM yet?

@Be-ing: There is still an open TODO in the description, do you plan to fix it? I do not understand how master write access will change the situation? We have discussed that no one should merge his own PRs without a formal LGTM. Thats why #894 is for example also not merged. Accepting PRs to private branches is IMHO a good technique to push a branch forward, that is not in a mergable state.

@Be-ing
Copy link
Contributor Author

Be-ing commented Nov 14, 2017

Sorry for not following all details of this branch an merge it in a time.

It's not your fault. It should not be any one person's responsibility to follow every pull request in detail.

There is still an open TODO in the description, do you plan to fix it?

Yes, I do. The master level meter with 4 decks parallel waveforms is not the same height as the deck meters, but I don't think that's necessary to fix before merging. We could go on forever improving this but it has been in a good mergeable state that is better than master for weeks. For the future let's separate TODO lists into tasks that must be fixed before merge and those that don't need to be fixed before merge so this is clearer.

We have discussed that no one should merge his own PRs without a formal LGTM. Thats why #894 is for example also not merged. Accepting PRs to private branches is IMHO a good technique to push a branch forward, that is not in a mergable state.

I agree that we generally shouldn't merge our own PRs. @ronso0, @uklotzde, and @sblaisot have tested this and should have been able to merge it too. PRs for private branches are good for branches that aren't in condition to merge to master, but this has been for a while. These absurdly long delays have been happening repeatedly, especially for skin PRs, and all it does is frustrate users and make it more annoying to do development.

@ronso0
Copy link
Member

ronso0 commented Nov 14, 2017

@Be-ing A while ago you asked me to help you fix Deere because you expexted to have little time for it.
I helped out and opened PRs against this (your) branch since you somehow took over design and UX leadership for Deere with the previous Deere update PR. That -and only that- is the reason why I opened the PRs against your fork rather than master so you can test them and merge with this PR if you find the changes worth. There's still one ToDo open in the OP of this PR and you didn't ask for final approval of the changes done so far. Furthermore you joined the discussion about redesigning the knobs I started here. If you don't like that procedure, please let me know earlier.

@ronso0
Copy link
Member

ronso0 commented Nov 14, 2017

I think for skin PRs like this we should generally make sure that they were tested on all platforms before merge, maybe by checkboxes in the OP. See previous glitches in EffectSelector and beat spinboxes.

@Be-ing
Copy link
Contributor Author

Be-ing commented Nov 14, 2017

@ronso0, you've done nothing wrong by sending me PRs and I apologize if it came off that way. My point is this branch should've been merged to master weeks ago and your recent PRs should have been able to be targeted at master.

@Be-ing
Copy link
Contributor Author

Be-ing commented Nov 15, 2017

I think for skin PRs like this we should generally make sure that they were tested on all platforms before merge, maybe by checkboxes in the OP. See previous glitches in EffectSelector and beat spinboxes.

That would be ideal, but I don't think we have enough people testing on every platform (especially macOS) to make that a requirement for merging a PR. It should certainly be a requirement for a release though.

@ronso0
Copy link
Member

ronso0 commented Nov 15, 2017

IMO this is ready to merge. On Linux it's fine, @sblaisot tested it for Windows I suppose. Did anyone do so for OSx?
@Be-ing If we want to fix the remaining issues I'd like my layout fixes PR to be tested and merged, instead of patchworking with PRs for every little skin issue.

I'll re-open the knob redesign PR against master edit: as soon as this merged (easier than rebasing).
Right now it seems to be a consensus to drop LateNight and Shade as long as they offer neither the new effects nor the new looping UI. So it seems very likely Deere will be the default skin and I think we can do better than what we have in the current design.

@Be-ing
Copy link
Contributor Author

Be-ing commented Nov 15, 2017

I did not explicitly ask for merging this before because I am exhausted from having to repeatedly ask one person for every little change. This cannot be one person's responsibility if we are going to have releases more often than every two years. We have "continuous integration" services, but what we practice is the opposite of continuous integration.

@ronso0
Copy link
Member

ronso0 commented Nov 15, 2017

Okay, I somehow understand your frustration and it's great you started that email discussion about it.
For now we have to live with it, I guess. No matter how many members are able to merge, skin PRs like this are practically impossible to safely review code-wise, members have to rely on trusted reviewers to test as many skin configurations as possible on various OS. No big deal to request LGTMs from users and then ping @daschuer to merge.

@Pegasus-RPG
Copy link
Member

@esbrandt Are you able to take a look at this?

@uklotzde uklotzde added this to the 2.1.0 milestone Nov 25, 2017
@uklotzde
Copy link
Contributor

@Be-ing There is still one open task on your check list. Do you plan to finish it here? Otherwise I would like to merge this PR and the polishing can be done afterwards. We already have #1399 as a follow up.

Great job 👍 The UI of Mixxx is evolving at an impressive speed. I hope we can pick up this momentum for other parts of the application, too! We definitely need more domain experts in all areas.

@ronso0
Copy link
Member

ronso0 commented Dec 1, 2017

@uklotzde I fixed the mixer layout for all configurations.
Quoting @Be-ing here the only tiny 'issue' left is

The master level meter with 4 decks parallel waveforms is not the same height as the deck meters, but I don't think that's necessary to fix before merging.

I think this is ready to merge, too.

@uklotzde
Copy link
Contributor

uklotzde commented Dec 1, 2017

The channel meters are tiny in the full featured 4-deck parallel layout, anyway. Having a bigger master level meter instead of wasting space might be desirable, even if it doesn't match the scaling of the channel meters. We might find a compromise or even new ideas for different options afterwards.

LGTM.

@uklotzde
Copy link
Contributor

uklotzde commented Dec 1, 2017

Merging this now since no one voted against it. The beta release date is near and we should get feedback from some alpha testers about our main skin.

@uklotzde uklotzde merged commit 81eea27 into mixxxdj:master Dec 1, 2017
@naught101
Copy link
Contributor

New interface is 👌

Thanks so much for your work @be, @ronso0, and others!

@Be-ing Be-ing deleted the deere_2.1_part_2 branch December 5, 2017 23:58
@sblaisot
Copy link
Member

Deere 2.1 menu has not been corrected. Still squares.
image

@Be-ing
Copy link
Contributor Author

Be-ing commented Dec 20, 2017

Pull requests welcome.

@JosepMaJAZ
Copy link
Contributor

JosepMaJAZ commented Dec 20, 2017 via email

@sblaisot
Copy link
Member

Well. the problem is bigger than that. We explicitely load a font. so we shouldn't rely on fallback font using characters that are not embedded in these fonts.

for me, we have 2 solutions:

  1. Use characters that are included in the font. I tried but didn't find suitable characters
  2. change font

@sblaisot
Copy link
Member

Pull requests welcome.

I know that :)
that was more an informational message

@JosepMaJAZ
Copy link
Contributor

JosepMaJAZ commented Dec 20, 2017 via email

@sblaisot
Copy link
Member

Yeah, that works ! Thanks. PR #1430

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants