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

Provide status messages on splash screen when loading (#1696) #1915

Merged
merged 2 commits into from Apr 27, 2015

Conversation

Wallacoloo
Copy link
Member

Based on the discussion about #1696, this updates the splash screen to look like this:

lmms-splash-with-progress

No progress bars or anything, just a discreet status message in the lower-left that updates to reflect the current component being initialized.

@Spekular
Copy link
Member

Spekular commented Apr 2, 2015 via email

@Wallacoloo
Copy link
Member Author

Glad you like it :-)

@@ -39,8 +42,9 @@ class Song;
class Ladspa2LMMS;


class EXPORT Engine
class EXPORT Engine : public QObject
Copy link
Member

Choose a reason for hiding this comment

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

FYI - @diizy may object here to using signals and slots on a non-GUI class.

@tresf
Copy link
Member

tresf commented Apr 2, 2015

This looks really good, thanks!.

Per @diizy there have been some strategic decisions to not add QObject references to non-GUI components, so this might need some redesign before it is accepted.

@M374LX
Copy link
Contributor

M374LX commented Apr 2, 2015

Really nice idea.

@Wallacoloo
Copy link
Member Author

I won't argue here with the decision to avoid signals (I don't actually disagree with it), but if there is an objection to using signals then please recommend an alternative.

m_projectNotes = new ProjectNotes;
displayInitProgress(tr("Preparing beat/baseline editor"));
Copy link
Member

Choose a reason for hiding this comment

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

A typo here, its beat+bassline editor.

Copy link
Member

Choose a reason for hiding this comment

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

*it's

:)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for catching that! Should be fixed now.

Copy link
Member

Choose a reason for hiding this comment

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

:)

@tresf
Copy link
Member

tresf commented Apr 2, 2015

but if there is an objection to using signals then please recommend an alternative.

This isn't my area of expertise. I don't know the best approach but until we settle on one, we may want to consider decoupling the status messages from the Engine class and simply loose that display information for now.

@Wallacoloo
Copy link
Member Author

That is one option. Another option is to pass an (optional) std::function to Engine::init that serves as a progress callback. This requires C++11 or boost; I don't know what C++ version is currently being targeted. Alternatively, we could use classic C static function pointers for the callback.

Also possible is to split the Engine::init() function into smaller pieces that must be called successively: Engine::initWavetables(), Engine::initPluginFileHandling(), etc. This allows the GuiApplication to insert progress messages at each step. The downside is that it doesn't adapt to future changes in the Engine init system.

Another possibility is to do the initialization in a separate thread from the main GUI and use some sort of messaging system, say condition variables for communication. A benefit to this is that it's much easier to create a responsive UI during initialization using a separate init thread. That is, it more effectively solves the problem of the splash screen not being dismissable by clicking, and potentially allows things like loading animations or for progress bars that update smoothly. I'm not too worried about that, but it's a possibility. Personally, I think this last option is overly complicated.

@Wallacoloo
Copy link
Member Author

I won't argue here with the decision to avoid signals

Well I may have lied there. Is it really a problem to inherit from QObject in the core module? That would make sense to me if there was a goal to remove the Qt dependency from the core, but looking through the rest of the code, and reading some discussions here on github, it seems that the issue is only using QtGui resources in the core, and relying on slots for real-time functionality.

Since initialization messages aren't real-time critical events, I don't see what's wrong with using signals here.

@tresf
Copy link
Member

tresf commented Apr 4, 2015

I won't argue here with the decision to avoid signals... Well I may have lied there.

Good. :)

@diizy's input is much more valuable than mine in this instance. Hopefully he has some time to chime in. 👍

@softrabbit
Copy link
Member

If there are rules that developers are supposed to follow, shouldn't they be documented somewhere easier to find than in the comments for some issue (that might seem unrelated at first glance)?

Not that I'm questioning the rule, whatever it is. I did a fair bit of googling on this, and didn't find much concrete info either way. But I'm sure @diizy has sources to back up his policies.

One would imagine using signals and slots in function A doesn't affect function B, so if that is the unclean thing being targeted the rule could really be applied with finer granularity than on a class basis. OTOH, inheriting QObject means at least some automagic stuff is happening behind the scenes, and that might taint the whole class. OT3rdH, static functions, like in Engine, might be different as they don't involve an object of the class...

@Wallacoloo
Copy link
Member Author

#1982 describes an alternative implementation of splash screen status messages that avoids using signals or inheriting from QObject.

I should stress that it's an alternative to this PR, as opposed to outright superseding it, because nobody had provided concrete feedback on whether signals should absolutely be avoided in this case and/or what a good alternative would be, and some may prefer the signal version because it is simpler (though personally, I do think 1982 is a better implementation).

@tresf
Copy link
Member

tresf commented Apr 27, 2015

If there are rules that developers are supposed to follow, shouldn't they be documented somewhere easier to find than in the comments for some issue (that might seem unrelated at first glance)?

Yes, not only for the RT-stuff but probably for the software in general. If a new developer were to get involved, it would be nice to hand them a good architectural map with some design flow, FAQs.

static functions, like in Engine, might be different as they don't involve an object of the class...

👍

@Wallacoloo this seems to be the preferred approach, merging. Thanks for taking the time to make two PRs.

tresf added a commit that referenced this pull request Apr 27, 2015
Provide status messages on splash screen when loading (#1696)
@tresf tresf merged commit 6428c7a into LMMS:master Apr 27, 2015
@Wallacoloo
Copy link
Member Author

@tresf I was literally just typing out a comment about going with your earlier suggestion to just remove the bits from the engine until we get a better message passing interface in LMMS. Are you sure you're comfortable merging this as-is?

@tresf
Copy link
Member

tresf commented Apr 27, 2015

@tresf I was literally just typing out a comment about going with your earlier suggestion to just remove the bits from the engine until we get a better message passing interface in LMMS. Are you sure you're comfortable merging this as-is?

Yes. The RT-safety and abolish all signals/slots is an initiative that has merits in DSP-heavy code and AFAIK this code is really just firing up the singleton processing engine. If certain parties want to make abolish all signals and slots a big deal, 25 days is plenty of time to be the 12th juror, so to speak. :)

@Wallacoloo
Copy link
Member Author

@tresf great point. I'm glad this feature is finally seeing the light of day!

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

6 participants