stefanha/wahjam — comment on commit 961f2fd04b5a2fab5ee964b8ce1cc485b9a17bfd
File:  line 0
By stefanha:

On Mon, Feb 13, 2012 at 2:20 AM, tea

wrote:
> # BeatInfoChanged slots has direct access to metronome
>
> I suggest use Qt 's signals/slots.
>
> (in this metronome case)
> If applyed this suggestion, the variable "metronomeBar" will be used only within the MainWindow constructor
> and the all tasks what MainWindow side does for metronome object are:
>
> * Create and Initialize the instance
> * Connect signals/slots
> * Add widget to layout

MetronomeBar does not need to know about bpm, only bpi. The
BeatInfoChanged signal provides both values, so either MetronomeBar
would have to discard bpm or ClientRunThread would have to split this
signal into two signals (one for bpi change, one for bpm change).

Right now MainWindow updates the BPM/BPI status bar labels and then
forwards the new bpi on to metronomeBar.

I can try this out.

> ## Method like a "reset()" is often slot in other Qt widgets
>
> Here, the mean such like method work like reset(), e.g QTextEdit::clear().
> The reset() call can be signal/slot or called internal in metronome bar setBeatsPerInterval slot.

Yes, good idea.

> ### The behavior for BPI/BPM changed event
>
> windows client does not reset current beat position when bpi/bpm are changed.
> they count the rest of the loop, and update BPI/BPM on position at 0.
>
> Can remove the reset() on BeatInfoChanged slots ? or is that still needed for BPM changes ?

The bpm/bpi does not change immediately because the BeatInfoChanged
signal is raised only when the next interval starts. We have the same
behavior as the Windows client.

The reason why I have reset() is because Disconnect() calls
BeatInfoChanged(0, 0). This causes the status bar BPM/BPI labels to
say "N/A" and the metronome bar resets.

I think the reset() could be moved into the if (bpi > 0) else
statement, so it only happens when BeatInfoChanged(0, 0) is called.

> ## setBeatsPerInterval is a slot
>
> That can be connected by something bpi changed notify signal, instead of call the method manually.
> now the arguments does not match.
>
> * the signal arguments are bpi and bpm
> * metronome slot recieve bpi only
>
> This change may require add new signal for bpi changed notification, but it does not be much cost.

Okay, I commented on the same issue above.