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

Skin layout revamp dev #639

Closed

Conversation

ferranpujolcamins
Copy link
Contributor

Summary of changes:
-Removed the layout option buttons of the top of LateNight
-Added layout options to view menu. Rearranged view menu.
-Added the option to hide the crossfader to LateNight and Shade
-Modified LateNight so that the library cannot be hidden when the skin is in medium or full size.

rryan and others added 30 commits April 22, 2015 17:37
…onBuild

Fix Windows build (SoundSourceMediaFoundation)
mp3: Workaround for different sample rates in on file
SoundSourceProxyTest.seekForward was failing on OSX due to
cover-test.mp3 not producing accurate samples on a seek. It seems this
is related to the MP3 decoder not being "primed" right after a seek. To
compensate for this, we seek backwards 29 MP3 frames (same as in
SoundSourceMp3) and read forward.

The test passes now but it would be ideal if we had a reliable way to
determine the right amount to read on a per-file basis from ExtAudioFile
directly (in case other formats need pre-fetching). I'm following up
with Apple support for help here.
Add "stabilization read" to MP3 seeks in SoundSourceCoreAudio.
The decoding functions do NOT return the actual error code!! Instead
the error code must be read from mad_stream if the decoding functions
returned != 0.

This caused some really ugly sounds at the beginning of some MP3 files,
because frames were not skipped as suppossed to be.
Slightly more code, but hopefully easier to understand.
*** CID 61970:  Uninitialized members  (UNINIT_CTOR)
/src/sources/soundsourcesndfile.cpp: 17 in Mixxx::SoundSourceSndFile::SoundSourceSndFile(QUrl)()
11         return list;
12     }
13
14     SoundSourceSndFile::SoundSourceSndFile(QUrl url)
15             : SoundSource(url),
16               m_pSndFile(NULL) {
>>>     CID 61970:  Uninitialized members  (UNINIT_CTOR)
>>>     Non-static class member field "m_sfInfo.seekable" is not initialized in this constructor nor in any functions that it calls.
17     }
18
19     SoundSourceSndFile::~SoundSourceSndFile() {
20         close();
21     }
*** CID 61968:  Uninitialized members  (UNINIT_CTOR)
/src/sources/soundsourcemp3.cpp: 163 in Mixxx::SoundSourceMp3::SoundSourceMp3(QUrl)()
157               m_fileSize(0),
158               m_pFileData(NULL),
159               m_avgSeekFrameCount(0),
160               m_curFrameIndex(kFrameIndexMin),
161               m_madSynthCount(0) {
162         m_seekFrameList.reserve(kSeekFrameListCapacity);
>>>     CID 61968:  Uninitialized members  (UNINIT_CTOR)
>>>     Non-static class member field "m_madSynth.pcm" is not initialized in this constructor nor in any functions that it calls.
163     }
164
165     SoundSourceMp3::~SoundSourceMp3() {
166         close();
167     }
…undSources

Fix coverity warnings for new SoundSources
@ywwg
Copy link
Member

ywwg commented Jul 10, 2015

I want to start to work on other non-related skin improvements.

I think this set of improvements is fine for this PR. we don't have to solve all the problems now

@Be-ing
Copy link
Contributor

Be-ing commented Jul 10, 2015

One option is to make a generic FX chain template that can be applied to all skins with minor color theming.

This seems like it could be a good idea to unify the effects UI. The variability between skins now is awfully confusing and unintuitive and puts controller mappers in an awkward situation.

@ferranpujolcamins
Copy link
Contributor Author

I've just merged 1.12.

The only bug I see is that In the larger sizes, the library button is clickable but doesn't do anything.

It's a workaround for another bug (worse in my opinion). As I commented before:

<<<
By the way, there's one problem: when on med or full mode, the show library CO does nothing. It behaves like this for two reasons:
1-It didn't make sense to be able to hide the library if the space was to be unused anyway.
2-The show library CO is a trigger for a widget stack when LateNight is in small mode.
If the last time LateNight was in small mode it had the sampler page selected (or fx or mic page), on the next Mixxx run, the show library CO is set to 0 because the selected tab is another one. This happens even if LateNight is in med mode, because the fx_samplers_container_small template is hidden but is still there. What this caused is that if LateNight was in med or full mode on Mixxx start, the library was hidden even if it was visible on the last Mixxx run. Disconnecting the library from the show library CO in med and full mode makes this bug disappear."

I think the clock is fine where it is, but it might also work in the right corner along with CPU. Up to you.

I like it in the center :)

@ferranpujolcamins
Copy link
Contributor Author

Oh, I've just thought I could change the color or the "L" and "R" of the crossfader assign button to red and blue, it's a quick fix. The user could see at a glance where is a deck assigned. Would you like that?

@ferranpujolcamins
Copy link
Contributor Author

Another possible quick fix: can I get rid of the striped backgrounds on the decks and samplers? I don't really like them .

@ywwg
Copy link
Member

ywwg commented Jul 11, 2015

The user could see at a glance where is a deck assigned. Would you like that?

leave that alone for now, we use red and blue for deck 1/2 vs 3/4 so it would be confusing to also use red/blue for right/left. Or just make both the R and L yellow text -- there's no real need to color code.

Please put it back so the library button is not clickable -- it makes more sense than to have the button be broken. Or figure out a way to skin it so it is always on but has a "disabled" (unclickable) appearance. Like, flat with grayer text.

@ferranpujolcamins
Copy link
Contributor Author

Please put it back so the library button is not clickable -- it makes more sense than to have the button be broken. Or figure out a way to skin it so it is always on but has a "disabled" (unclickable) appearance. Like, flat with grayer text.

I can do that we will still have the same problem with the view menu check option. I think this can't completely be solved until we have a way to communicate to Mixxx which CO a skin offers.

@ywwg
Copy link
Member

ywwg commented Jul 11, 2015

how is it that in Deere, the menu items are grayed out?

can I get rid of the striped backgrounds on the decks and samplers

no thanks :). Maybe in the future I'll change my mind but let's leave them for now.

@ferranpujolcamins
Copy link
Contributor Author

On a skin reload, mixxx.cpp checks which of these CO are declared by the skin. It was already this way.

@Be-ing
Copy link
Contributor

Be-ing commented Jul 19, 2015

How is progress on this?

@ferranpujolcamins
Copy link
Contributor Author

Hi, I think the only thing that still needs to be done is this:

Please put it back so the library button is not clickable -- it makes more sense than to have the button be broken. Or figure out a way to skin it so it is always on but has a "disabled" (unclickable) appearance. Like, flat with grayer text.

I'll try to fix this tomorrow.

@ferranpujolcamins
Copy link
Contributor Author

Done. What else is blocking this?

@Be-ing
Copy link
Contributor

Be-ing commented Jul 24, 2015

Could you make the crossfader assignment CO persistent? As an example use case, I may want to keep decks 1 & 2 assigned to left and right, but not assign 3 & 4 to the crossfader. This would allow me to use the crossfader and two channel faders on my controller without getting confused when toggling between decks 1/3 and 2/4.

@Be-ing
Copy link
Contributor

Be-ing commented Jul 24, 2015

I am opposed to merging this until https://bugs.launchpad.net/mixxx/+bug/1462061 is fixed. Without the ability to hide the library, that bug can't be worked around.

@Be-ing
Copy link
Contributor

Be-ing commented Jul 24, 2015

This is still a bit too tall for my screen 1366x768 laptop screen. The bottoms of the buttons on the bottom bar are cut off by my panel when not in fullscreen mode.
late-night-pr-screenshot

@ferranpujolcamins
Copy link
Contributor Author

Hi @Be-ing , Shrinking the height of LateNight is yet another thing I'm planning to do.
I'm investigating the bug you've mentioned as well.

@daschuer
Copy link
Member

daschuer commented Aug 3, 2015

What is current state of this? I am afraid the git branch is messed up since there are unrelated commits listed, and it is not in a merge-able state. Is it still a 1.12 candidate?

@ywwg
Copy link
Member

ywwg commented Aug 3, 2015

Is it still a 1.12 candidate?

No, let's keep this master-only. I think LateNight is in fine shape for 1.12.

@ywwg
Copy link
Member

ywwg commented Aug 3, 2015

(to be specific, the code changes and changes to other skins required by this alteration, coupled with the "nice-to-have" nature of the changes tip it in my mind to something for master. Although the height issue in latenight is annoying, it's not really a blocker)

@ferranpujolcamins
Copy link
Contributor Author

In fact this doesn't solve the issue of LateNight being too tall anyway.
It's true that this was a first step towards a solution (hide/show
crossfader), but it has mutated to a layout options re-factoring.

We still need to make a lot of work in order to offer the user a complete
("all" functions available in each skin), consistent (skins can't have a
very different work-flow for common tasks such as beat-looping, fx, or the
layout options discussed in this PR), polished (that endless list of
little things that can look and work better) and beautiful experience
with skins. Don't worry we will eventually make it for 1.13 or whatever
release.

This is going to be long, so let's just take a compromise on each of the
skin aspects above for now. Let's just try to give birth to 1.12 ASAP :)

2015-08-03 14:46 GMT+02:00 Owen Williams [email protected]:

(to be specific, the code changes and changes to other skins required by
this alteration, coupled with the "nice-to-have" nature of the changes tip
it in my mind to something for master. Although the height issue in
latenight is annoying, it's not really a blocker)


Reply to this email directly or view it on GitHub
#639 (comment).

@Be-ing
Copy link
Contributor

Be-ing commented Jan 19, 2016

@ferranpujolcamins, are you planning on finishing this PR for 2.1?

@ferranpujolcamins
Copy link
Contributor Author

Sure, I have some free days. I'll take a look.
El dia 19/01/2016 4:58 a. m., "Be" [email protected] va escriure:

@ferranpujolcamins https://github.com/ferranpujolcamins, are you
planning on finishing this PR for 2.1?


Reply to this email directly or view it on GitHub
#639 (comment).

@ferranpujolcamins
Copy link
Contributor Author

Well, actually not. Sorry.

2016-01-19 8:22 GMT+01:00 Ferran Pujol Camins [email protected]:

Sure, I have some free days. I'll take a look.
El dia 19/01/2016 4:58 a. m., "Be" [email protected] va escriure:

@ferranpujolcamins https://github.com/ferranpujolcamins, are you
planning on finishing this PR for 2.1?


Reply to this email directly or view it on GitHub
#639 (comment).

@ferranpujolcamins
Copy link
Contributor Author

Superseded by #913, which merges to master cleanly.

@ferranpujolcamins ferranpujolcamins deleted the skin-layout-revamp-dev branch May 14, 2019 16:22
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.