Can this have a name other than update? The base class function isn't virtual, so this function shadows it, not overrides it. I don't know how this interacts with the signal-slot mechanism, but it may not behave as the implementation/callers intended. Also, it needn't be marked virtual, as it doesn't look like it's intended to be overridden.

I did not even think of any of this, I just copied it from LcdSpinBox and assumed it will work the same. I would guess it still needs to be called update() since that is what Qt calls when it plans to redraw a widget?

Since QWidget::update() is non-virtual, the name of this function doesn't matter.

I know it doesn't matter, but I think the name is misleading, shadowing QWidget::update with something different. Also, slot lookups are done using strings, not vtables, so it's still not clear how this interacts with the signal-slot mechanism. I'll investigate it.

To clarify, I was answering to @he29-net's question.

I just tried to rename it and the display GUI stopped updating. So Qt (or LMMS, not sure who calls the global update() on all widgets) definitely expects the slot to be named update().

I tested this, see the code at https://gist.github.com/DomClark/fc976514c6e1b083d9bce0ab777eefad. It appears slots have virtual behaviour when connected using SIGNAL/SLOT macros, and non-virtual behaviour when using member function pointer connections. This inconsistency, along with the fact that it doesn't work with the modern connection style, suggests to me that it's not a good idea to override slots by name. I'm fine with leaving it as is for now, since the existing LcdSpinBox code does the same thing, but I think we should clean this up in the future.

That inconsistency certainly looks like an invitation for some confusing bugs, interesting..

I wonder what is the intended workflow then? I would expect that a derived widget implements its own update(), so that it can react to any changes specific to the modified version. And as a part of its update() it would call update() of the base class to finish the rest. So the non-virtual behavior should be almost always unwanted, right?

But if update()s are called by Qt on some occasions, wouldn't choosing a different "update" slot name on a derived class mean that it simply won't be called when needed? Or do we only call / signal update()s explicitly from our code?

update() isn't supposed actually update anything itself. Its purpose is to schedule a repaint of the widget in the near future, once control returns to the event loop. If virtual behaviour were intended, it would be a virtual function.

It looks like the code there is to set the LCD display to the current value of the model, in which case it should run in response to changes in the model. The appropriate places for this are in LcdFloatSpinBox::modelChanged (called when a new model is viewed by the spin box) and in response to the Model::dataChanged signal (emitted when the model's value changes).

In addition to @DomClark's comment, ModelView::doConnections connects Model::dataChanged and Model::propertiesChanged to SLOT(update()) of ModelView::m_widget. This means at least one of them should be satisfied:

• paintEvent() of the model reads the value of the model,
• The view monitors changes in its model
• The view implements its own update()

LcdFloatSpinBox(and LcdSpinBox, too) is using the third approach. If this approach is not recommended, we should move to other approaches.
However, this looks out of scope of this PR.

Could call it updateDisplay as it's basically what the method is doing.

Maybe we should add a TODO comment for futurely moving the border drawing to the LcdSpinBox class later. It's inconsistent that on regular spin boxes the border is drawn on the LcdWidget class and on the float spin boxes it's drawn on the LcdFloatSpinBox paint event.

If it's possible to move this to the LcdSpinBox class with no major issues in this PR, I'd even think better to just deal with it now.

I'm not sure if I read it correctly, but wouldn't that mean that all "vanilla" LcdWidgets would have no borders?

No, I just meant that on the regular LCD Widget, the borders are being drawn on the LcdWidget class (LcdWidget::paintEvent, where this comment was made) but on the Float LCD Widget they are being drawn on the LcdFloatSpinBox class (LcdFloatSpinBox::paintEvent). As far as I understood, LcdWidget is the LCD number while the LcdSpinBox is the widget itself, that contains the LCD number. LcdFloatSpinBox for example has 2 LcdWidgets.

I was just pointing out it feels inconsistent to draw borders on the LcdWidgets on the regular spin box and on the LcdFloatSpinBox for the float spin box. We probably should do the border drawing on the spin box classes (both LcdSpinBox and LcdFloatSpinBox).

We probably should do the border drawing on the spin box classes (both LcdSpinBox and LcdFloatSpinBox).

But that would mean LcdWidget is left with no border-drawing code, hence my worry that LcdWidgets will not have borders if we do that. They are currently used in the FX mixer (where a nuber is needed, but you cannot change it), so there would be no borders around the green numbers on the FX mixer. Either that, or I'm getting really confused. :D

LcdFloatSpinBox does border drawing simply because it needs to visually join two widgets that were not originally designed to be joined. That's also why there are the new "seamless" parameters in LcdWidget. It all feels a bit like a hack, all the three LCD widgets could be probably rewritten in some way to make it cleaner, but I felt that is already way out of scope of the original Microtuner (I generally try to avoid touching existing code when not necessary, not to mention big rewrites like this).

Oh, I didn't realize that LcdWidget was used by other widgets besides the Spin Box. Well, in that case it's probably a big rewrite to move the border to the outer widgets: Even though it makes sense it's out of the scope of this PR. I'm not sure it's worth it adding a TODO comment for the future, but you can ignore this comment then!

Could a comment be added at the beginning about the border drawing, just for future reference? Something like:

// When either the right or left is seamless we are in a LcdFloatSpinBox
// and the border drawing will be done by the float spin box class instead
// TODO: Possibly move the frame drawing to the outer widget classes
// (i.e.: Spin boxes) for consistency

I know this sounds unimportant, but if in the future someone decides to change the frame drawing and isn't aware of that, the frames of regular spin boxes and float spin boxes won't match. The TODO also serves as a reference for possibly coming up with an alternative in the future.

I added a comment explaining why is the border drawing skipped. I left out the "TODO", because I still do not understand where do you propose to "move" the code. If it is moved, it is no longer there, which breaks LcdWidget borders in the mixer. So we can only copy it to LcdSpinBox, which does not make sense, it only adds more duplicate code that needs to be maintained.

I don't know, maybe someone else can step in to give an opinion too, I could be wrong after all. For me it feels like bad design that a widget contains another one (SpinBox contains LcdWidgets) but in some cases the inner widget (LcdWidget) draws the frame and in other cases the outer widget (Float Spinbox) draws the frame. In a ideal scenario either the inner OR the outer widget should be responsible by the frame. That's all I was suggesting, that in the future we move the drawing of the frame to the outer widgets (LcdFloatSpinBox, LcdSpinBox, FxLineLcdSpinBox, etc) or that we come up with a design where we don't have it being drawn in different places.

I understand this is out of the scope of this PR (and obviously not its fault by any means), I just thought about adding a TODO comment so we could remember about maybe fixing it in the future. Just to let you know, this is by no means holding my approval (as I mentioned in Discord, I'm mostly waiting on @DomClark regarding the update method name, the aside the PR LGTM).