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

# method naming reset()

I worried the naming conflict if bass classes used.
That was no worry QObject or QWidget both does not use the name for method.
Pass this topic, I just note here I searched about it.

# 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

## 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.

### 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 ?

## 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.

## Merit of these changes

This can decrease future changes cost. for example, when the metronome move to another class,
no need to search whole code, and the context of how other methods are used. Signals/Slots promises that interface.

NOTE: "scope of variable" may be incorrect term,
I did not mean a lifetime of the object. I mean just where the variable name appear on the file,
that defines how wide range developer has to look and take care for new changes.

----

# MetronomeBar::setCurrentBeat slot

No need range check ? (any case when setCurrentBeat get invalid value ?)
If no need or don't want the check every times, but assert macro still helps code readability.

Q_ASSERT(bpi >= currentBeat && currentBeat >= 0)

> This could be optimized to repaint only changed regions but it just makes the code trickier.

I see that can not take a straight approach for NJClient.
update signal by passing the range (prevBeat, currentBeat) may helps though,
but no need that optimization yet. leaving that comment is enough ok for now.

Current patch code has call update() twice at BPI change, that in reset() and setBeatPerInterval(bpi)
but the area of MetronomeBar update is not so wide and BPI change is rare event, I think performance won't be problem.
this can leave until when need optimize or cleanup code.