[patch] Use signals for communication between SequenceManager and TransportDialog

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[patch] Use signals for communication between SequenceManager and TransportDialog

David Faure
I add move-to-next-bar to my unittest, but it turns out that (unlike move-to-next-note) this requires
the SequenceManager, which in turn requires TransportDialog, RosegardenMainWindow etc.

So I'm doing a bit of rearchitecture around SequenceManager.
Here's a first step, maybe you want to review that design change?

--
David Faure, [hidden email], http://www.davidfaure.fr
Working on KDE Frameworks 5

------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Rosegarden-devel mailing list
[hidden email] - use the link below to unsubscribe
https://lists.sourceforge.net/lists/listinfo/rosegarden-devel

0001-Use-signals-for-communication-between-SequenceManage.patch (18K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [patch] Use signals for communication between SequenceManager and TransportDialog

David Faure
On Sunday 01 May 2016 11:15:19 David Faure wrote:
>
> +    void signalTempoChanged(tempoT);
> +    void signalMidiInLabel(const MappedEvent *event);
> +    void signalMidiOutLabel(const MappedEvent *event);
> +    void signalPlayButtonChecked(bool checked);
> +    void signalRecordButtonChecked(bool checked);
> +    void signalMetronomeButtonChecked(bool checked);

On second thought, this could of course clearly be improved.
This version is pure decoupling of classes, but actual decoupling
of concepts would lead to something like
 void playStatusChanged( some enum with playing, recording, whatever )

Or at least I could name these something like
  signalPlaying, signalRecording, signalMetronomeActive?

--
David Faure, [hidden email], http://www.davidfaure.fr
Working on KDE Frameworks 5


------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Rosegarden-devel mailing list
[hidden email] - use the link below to unsubscribe
https://lists.sourceforge.net/lists/listinfo/rosegarden-devel
Reply | Threaded
Open this post in threaded view
|

Re: [patch] Use signals for communication between SequenceManager and TransportDialog

D. Michael McIntyre-3
On 05/01/2016 05:55 AM, David Faure wrote:

>> +    void signalTempoChanged(tempoT);
>> +    void signalMidiInLabel(const MappedEvent *event);
>> +    void signalMidiOutLabel(const MappedEvent *event);
>> +    void signalPlayButtonChecked(bool checked);
>> +    void signalRecordButtonChecked(bool checked);
>> +    void signalMetronomeButtonChecked(bool checked);
>
> On second thought, this could of course clearly be improved.
> This version is pure decoupling of classes, but actual decoupling
> of concepts would lead to something like
>   void playStatusChanged( some enum with playing, recording, whatever )
>
> Or at least I could name these something like
>    signalPlaying, signalRecording, signalMetronomeActive?
>

It seems like more work than it's worth to completely decouple the
concepts, but I'm not doing the work, so that's your call.

I do like signalPlaying and friends better than signalPlayButtonChecked
and friends.

I looked over the proposed changes, and see no problem with this
approach.  The patch didn't apply cleanly, so I didn't build and test.
I'm basing this impression entirely on how it looks on paper, as it were.
--
D. Michael McIntyre

------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Rosegarden-devel mailing list
[hidden email] - use the link below to unsubscribe
https://lists.sourceforge.net/lists/listinfo/rosegarden-devel
Reply | Threaded
Open this post in threaded view
|

Re: [patch] Use signals for communication between SequenceManager and TransportDialog

David Faure
On Sunday 01 May 2016 08:38:22 D. Michael McIntyre wrote:

> On 05/01/2016 05:55 AM, David Faure wrote:
> >> +    void signalTempoChanged(tempoT);
> >> +    void signalMidiInLabel(const MappedEvent *event);
> >> +    void signalMidiOutLabel(const MappedEvent *event);
> >> +    void signalPlayButtonChecked(bool checked);
> >> +    void signalRecordButtonChecked(bool checked);
> >> +    void signalMetronomeButtonChecked(bool checked);
> >
> > On second thought, this could of course clearly be improved.
> > This version is pure decoupling of classes, but actual decoupling
> > of concepts would lead to something like
> >   void playStatusChanged( some enum with playing, recording, whatever )
> >
> > Or at least I could name these something like
> >    signalPlaying, signalRecording, signalMetronomeActive?
> >
>
> It seems like more work than it's worth to completely decouple the
> concepts, but I'm not doing the work, so that's your call.
>
> I do like signalPlaying and friends better than signalPlayButtonChecked
> and friends.

No problem I'll do the renaming, that's simple enough.

Is "metronome active" correct? I'm not sure what's the metronome functionality about in this context.

> I looked over the proposed changes, and see no problem with this
> approach.  The patch didn't apply cleanly, so I didn't build and test.
> I'm basing this impression entirely on how it looks on paper, as it were.

Oops I had one unpushed renaming that this was based on. Pushed now,
so it should apply cleanly if you still want to give it a try (I'm quite confident that
it doesn't introduce any regression though).

--
David Faure, [hidden email], http://www.davidfaure.fr
Working on KDE Frameworks 5


------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Rosegarden-devel mailing list
[hidden email] - use the link below to unsubscribe
https://lists.sourceforge.net/lists/listinfo/rosegarden-devel
Reply | Threaded
Open this post in threaded view
|

Re: [patch] Use signals for communication between SequenceManager and TransportDialog

D. Michael McIntyre-3
On 05/01/2016 09:04 AM, David Faure wrote:

> Is "metronome active" correct? I'm not sure what's the metronome functionality about in this context.

I think "activated" seems to cover it well enough.

> Oops I had one unpushed renaming that this was based on. Pushed now,
> so it should apply cleanly if you still want to give it a try (I'm quite confident that
> it doesn't introduce any regression though).

I saw that.  It should be fine now.  My only real concern was having a
patch out of sync because you left some critical bit out somewhere, and
it was a mild concern at that.  I see the point, your approach looks
fine, carry on good sir!
--
D. Michael McIntyre

------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Rosegarden-devel mailing list
[hidden email] - use the link below to unsubscribe
https://lists.sourceforge.net/lists/listinfo/rosegarden-devel