Lmms: Beat editor: clicking on a beat toggles the previous beat on/off

[tukkek]

I'm using a fairly large amount of beats on a small window (48 beat/12 bars) and the actual click area of each beat does not correspond to the beat itself as-draw. Instead, it often will toggle the beat to the left of it, even when I'm clicking it very obviously on the right beat.

OK just as I'm writing this I think I notice what's wrong. When I move the cursor on to the beat, if goes from the normal system cursor to a "pointing hand" and the actual cursor click does not register on the position of the pointing finger but on the thumb instead, which is a few pixels to the left.

Here's what I mean: even though the index finger is clearly on the right beat, the click registers on the thumb instead (left beat):

I'm not sure if that's my system's cursor too or something that comes with LMMS. Either way, it gets pretty confusing, especially when working with more beats in less screen space so an alternative would be great.

[tukkek]

I have tried resizing the window to get bigger beats and the problem still persists (this is how I took the screenshot above). I can't recall ever having a problem like this in previous versions of LMMS and I've used the beat editor a fair amount of times, as you'd imagine.

[tresf]

Yes, this is a mouse position bug but it appears worse on your computer, possibly due to the way the theme is rendering. Here's the code: https://github.com/LMMS/lmms/blob/stable-1.2/src/tracks/Pattern.cpp#L723

It appears we do some magic math to determine which step was clicked. I would recommend beginning debugging there. Any additional information about your computer is useful so that a developer can understand what UI differences would cause this, i.e. Anything that influences the spacing or positioning of items.

[PhysSong]

Reproduced. I guess this is related with cursor shapes. If the coordinate information in QMouseEvent doesn't point to the end of index finger, that could be an issue.

[zonkmachine]

If the coordinate information in QMouseEvent doesn't point to the end of index finger, that could be an issue.

I couldn't find where we set that. The places where we specifically set Qt::PointingHandCursor we don't specify the coordinates for the hot-spot and it works well anyway. Also the error changes with the number of beat steps and if the BBEditor is expanded or not so that suggests the algorithm is at play here. Could be a bit of both but I went ahead and tweaked the algorithm a bit. https://github.com/LMMS/lmms/pull/5466

[zonkmachine]

I was wrong. Everything is there in the issue description by @tukkek If you switch to the classic theme or copy over the old icon hand.png to the default theme the problem is gone. Closing #5466

[zonkmachine]

The mouse cursor is selected here:
https://github.com/LMMS/lmms/blob/e199f72686224011fe8da23b8db8befb5a0bd2b4/src/core/Track.cpp#L296
and possibly here too:
https://github.com/LMMS/lmms/blob/e199f72686224011fe8da23b8db8befb5a0bd2b4/src/core/Track.cpp#L612

If I change setCursor( QCursor( embed::getIconPixmap( "hand" ), 3, 3 ) ); to
setCursor( QCursor( embed::getIconPixmap( "hand" ), 6, 3 ) ); the default theme works well and the classic theme catches the bug.

Possible solutions:

1. Let both themes use the same mouse cursor.
2. Make cursor themeable. I'm looking into this.
3. Change to a Qt default cursor like Qt::PointingHandCursor

I'm thinking alternative 1 for 1.2.2 and looking into more advanced cursor themability for the master branch.

[Veratil]

Possible solutions:

1. Let both themes use the same mouse cursor.

2. Make cursor themeable. I'm looking into this.

3. Change to a Qt default cursor like `Qt::PointingHandCursor`

I'm thinking alternative 1 for 1.2.2 and looking into more advanced cursor themability for the master branch.

1. Add two Q_PROPERTY for the mouse hotspot that tells where it should be. EDIT: I realize now this falls under 2, but more specific. 😂

[tresf]

Good recommendation... the Q_PROPERTY will definitely be the most scalable for potential future mouse cursor customizations.

[zonkmachine]

Unfortunately I don't think I'll have time to look into this right now so I'm unassigning from the issue. I do think it would be a good fix for 1.2.2 though.

[Spekular]

Using the same cursor as a temporary fix should be as simple as copy + pasting this hand.png over this one, right?

[zonkmachine]

The fastest fix is the other way around. The old theme hand is in tune with the code. If you use the new one you'll get the same problem with the Classic theme unless you also adopt the code but this could create similar glitches on other actions where the cursor is used. I know I experienced this when I was testing the bug. If you hack this I'll help and test it.

[JohannesLorenz]

1. Add two `Q_PROPERTY` for the mouse hotspot that tells where it should be.

@Veratil I don't know what you mean. Where do you want to add two Q_PROPERTYs? Can you please make a draft branch and push it? Then I'd complete it to a working PR.

[PhysSong]

@JohannesLorenz Simply add some qproperty-key: value; pairs to stylesheet files and use them by Q_PROPERTY macros in some relevant classes.

[JohannesLorenz]

in some relevant classes.

Yes, but in which classes? Is the plan to inherit QCursor?

[Veratil]

1. Add two `Q_PROPERTY` for the mouse hotspot that tells where it should be.

@Veratil I don't know what you mean. Where do you want to add two Q_PROPERTYs? Can you please make a draft branch and push it? Then I'd complete it to a working PR.

Something like this. Not tested.

--- a/data/themes/default/style.css
+++ b/data/themes/default/style.css
@@ -671,7 +671,8 @@ TrackContentObjectView   {
    qproperty-textBackgroundColor: rgba(0, 0, 0, 75);
    qproperty-textShadowColor: rgba(0,0,0,200);
    qproperty-gradient: false; /* boolean property, set true to have a gradient */
-
+   qproperty-mouseHotspotX: 16;
+   qproperty-mouseHotspotY: 16;
    font-size: 11px;
 }

--- a/include/Track.h
+++ b/include/Track.h
@@ -205,6 +205,8 @@ class TrackContentObjectView : public selectableObject, public ModelView
    Q_PROPERTY( QColor textShadowColor READ textShadowColor WRITE setTextShadowColor )
    Q_PROPERTY( QColor BBPatternBackground READ BBPatternBackground WRITE setBBPatternBackground )
    Q_PROPERTY( bool gradient READ gradient WRITE setGradient )
+   Q_PROPERTY(int mouseHotspotX MEMBER m_mouseHotspotX)
+   Q_PROPERTY(int mouseHotspotY MEMBER m_mouseHotspotY)

 public:
    TrackContentObjectView( TrackContentObject * tco, TrackView * tv );
@@ -315,6 +317,8 @@ private:
    QColor m_textShadowColor;
    QColor m_BBPatternBackground;
    bool m_gradient;
+   int m_mouseHotspotX;
+   int m_mouseHotspotY;


    bool m_needsUpdate;
    inline void setInitialPos( QPoint pos )
--- a/src/core/Track.cpp
+++ b/src/core/Track.cpp
@@ -282,7 +282,9 @@ TrackContentObjectView::TrackContentObjectView( TrackContentObject * tco,
    m_textShadowColor( 0, 0, 0 ),
    m_BBPatternBackground( 0, 0, 0 ),
    m_gradient( true ),
-   m_needsUpdate( true )
+   m_needsUpdate( true ),
+   m_mouseHotspotX(0),
+   m_mouseHotspotY(0)
 {
    if( s_textFloat == NULL )
    {
@@ -293,7 +295,8 @@ TrackContentObjectView::TrackContentObjectView( TrackContentObject * tco,
    setAttribute( Qt::WA_OpaquePaintEvent, true );
    setAttribute( Qt::WA_DeleteOnClose, true );
    setFocusPolicy( Qt::StrongFocus );
-   setCursor( QCursor( embed::getIconPixmap( "hand" ), 3, 3 ) );
+   setCursor( QCursor( embed::getIconPixmap( "hand" ), m_mouseHotspotX, m_mouseHotspotY ) );
    move( 0, 0 );
    show();

@@ -609,7 +612,8 @@ void TrackContentObjectView::leaveEvent( QEvent * e )
 {
    if( cursor().shape() != Qt::BitmapCursor )
    {
-       setCursor( QCursor( embed::getIconPixmap( "hand" ), 3, 3 ) );
+       setCursor( QCursor( embed::getIconPixmap( "hand" ), m_mouseHotspotX, m_mouseHotspotY ) );
    }
    if( e != NULL )
    {

[JohannesLorenz]

Something like this. Not tested.

I'm trying to get this working now.

[JohannesLorenz]

I pushed it as a draft PR. The offset numbers are still wrong, but let's first check the relevant things.

I took your draft @Veratil . The only changes I had to do:

• I had to replace the MEMBER in Q_PROPERTY (MEMBER is Qt5 only) with the normal READ/WRITE.
• Calling setCursor in the CTOR does not work. The properties are set after the CTOR, so setCursor in the CTOR still uses 0 and 0. So I added a further setCursor into the update function, which is only ever executed once.

[JohannesLorenz]

And btw, many thanks for the draft @Veratil !

[JohannesLorenz]

Will be fixed by #5489 .