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

Consolidate midi config widgets & move them out of the core #2289

Merged
merged 4 commits into from
Sep 6, 2015

Conversation

Wallacoloo
Copy link
Member

This takes MidiAlsaRaw::setupWidget, MidiAlsaSeq::setupWidget, MidiApple::setupWidget, MidiOss::setupWidget, MidiDummy::setupWidget and MidiWinMM::setupWidget and implements them all through a generic MidiSetupWidget class.

This offers the advantage of removing a significant amount of duplicated code (thus making it easier to change the setup widget appearance for all midi backends) as well as further decoupling the core from the gui.

Behaviorally, nothing has changed except:

  • Previously, MidiAlsaRaw stored its device setting under the field ConfigManager::inst()->getValue("MidiAlsaRaw", "Device"). Now, the field is ("MidiAlsaRaw", "device"), using a lowercase "device" just like all the other backends. The consequence is that the user will lose their midi device setting the first time they open LMMS after applying this patch if using MidiAlsaRaw (very minor side-effect IMO).
  • Previously, MidiWinMM showed a "Settings for WinMM" configuration section, despite the section being empty. This section is no longer displayed (just like with MidiDummy).

For context, all of the code changed is just responsible for showing the below "Settings for ..." section:
lmms-midi-guicore

@Wallacoloo
Copy link
Member Author

By the way, src/core/audio/Audioxxx could also benefit from this same consolidation as a followup PR (there's even more duplicated code in those setupWidgets).

@Wallacoloo Wallacoloo force-pushed the midi-guicore branch 2 times, most recently from eced6e2 to 45bff92 Compare August 24, 2015 00:41
@tresf
Copy link
Member

tresf commented Aug 24, 2015

The consequence is that the user will lose their midi device setting the first time they open LMMS after applying this patch if using MidiAlsaRaw (very minor side-effect IMO).

We should really consider writing an upgrade path for 1.1 -> 1.2 for this via src/core/DataFile.cpp#L795

@Wallacoloo
Copy link
Member Author

@tresf doesn't that just apply to upgrading projects from one version of lmms to the next? Or does that apply to the .lmmsrc file as well?

@Wallacoloo
Copy link
Member Author

Also, you've got to sit back and admire the documentation for that file for a few seconds: (line 2)

/*
 *DataFile.cpp - implementation of class DataFile
 ...
 */

@tresf
Copy link
Member

tresf commented Aug 24, 2015

Yes, my mistake... this is loaded as just a plain XML Tree node via src/core/ConfigManager.cpp#L302... Probably an area we should consider improving on...

-Tres

@Wallacoloo
Copy link
Member Author

Ah, but it does have an upgrade path: src/core/ConfigManager.cpp#L109. I'll insert the appropriate logic there.

@tresf
Copy link
Member

tresf commented Aug 24, 2015

but it does have an upgrade paaaaaaaaaath

👍

@Wallacoloo
Copy link
Member Author

Newest commit supports an upgrade path & Travis is happy :-)

While I was adding a new deleteValue method to ConfigManager (to remove the old "Device" field), I also shoehorned in a fix that renames the "_class", "_attribute" and "_value" parameters used in value() and setValue() to not have the underscores. I hope nobody minds 😉


Off-topic: ConfigManager stores its values as QMap<String, QVector<QPair<QString, QString> > >. Why? Our xml structure looks vaguely like this:

  <MidiAlsaRaw device="default"/>
  <Midialsaseq device="default"/>
  <app displaydbv="0" configured="1" language="en" nomsgaftersetup="0" disablebackup="0" nommpz="0"/>
  ...

So the fields of any "class" (MidiAlsaRaw, Midialsaseq, etc) are stored as a list of (key, value) pairs (QVector<QPair<QString, QString> >) rather than an easier to manage QMap<String, String>. This would be justifiable if we needed to preserve parameter order or allow duplicates, but just by loading and closing LMMS, parameter order gets scrambled. And XML doesn't allow for duplicate attributes, much less does lmms have a use for them.

Unless somebody knows a good reason why we use the current internal representation, I'll go ahead and open a ticket for this.

@softrabbit
Copy link
Member

Unless somebody knows a good reason why we use the current internal representation, I'll go ahead and open a ticket for this.

Not sure if that's necessary, see #2109. Looks like a pretty complete rewrite of everything config related.

@Wallacoloo
Copy link
Member Author

Just rebased this against master (to allow for auto-merge). Pending any further discussion, I'll merge this PR myself sometime over the weekend - it's been open for 2 weeks now.

@tresf
Copy link
Member

tresf commented Sep 6, 2015

Sorry @Wallacoloo somehow I had missed the new commits. I left a comment about the project version comparison.

Next release candidate will be named 1.1.90
@Wallacoloo
Copy link
Member Author

@tresf not a problem! I've addressed your line note.

@tresf
Copy link
Member

tresf commented Sep 6, 2015

👍

Wallacoloo added a commit that referenced this pull request Sep 6, 2015
Consolidate midi config widgets & move them out of the core
@Wallacoloo Wallacoloo merged commit 8a4e59d into LMMS:master Sep 6, 2015
@Wallacoloo Wallacoloo deleted the midi-guicore branch September 6, 2015 18:12
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.

3 participants