Hydrogen: Hydrogen is crashing when I change resolution

[KEvl235]

everytime I change the resolution, if I click on the zone for where to put midi notes hydrogen is crashing.

Am I the only one with this behavior ?

[mauser]

Hi!

Which version of hydrogen are you using? At the moment i can't reproduce your problem..

[KEvl235]

hello,

I tried to change from 8 to 16 for the resolution and it's crashing if I click on the grid. I am using the hydrogen-git from the Aur of archlinux

[SpotlightKid]

I just rebuilt the hydrogen-git package and I can't reproduce this crash either.

[trebmuh]

Fresh rebuild from https://github.com/hydrogen-music/hydrogen/commit/5556c8dbc2a2c58f1441281739eab444273861d0 on a Debian Stretch up to date and I can't reproduce the issue neither.

@hypsinom : is that still the case for you if you are rebuilding it?

[KEvl235]

I change from 8 to 16, I click. And Hydrogen crash. It was built on the aur ( arch )

[trebmuh]

@hypsinom : does it prints anything when crashing if you start hydrogen from a terminal?

[pvint]

I have reproduced this exactly on Gentoo (and it happens for me every time). Since I am the one who can reproduce it I will try to sort it out.

(See also closed issue #579 where I reported the same thing last night)

[pvint]

I have this narrowed down but am having some trouble understanding it fully thus far.

It appears to be related to changes made some time ago for QT5 compatibility. When I test using a clone from ~2 years ago the problem does not exist.

Here is what I have found:
When resolution_combo->getText() is called it returns the value prefixed by "&" whereas older versions did not.

Example in src/gui/src/PatternEditor/PatternEditorPanel.cpp:

                                __resolution_combo->set_text( "16" );
                                INFOLOG(__resolution_combo->getText());

Returns:
&16

Same test in older version (compiled on same system) returns:
16

When the "&16" is parsed by QString::number it sets the resolution to zero resulting in the strange behavior.
@mauser @trebmuh Any thoughts?

FWIW, I'm on QT 5.9.2 but I also have 4.8.7 libraries installed

[trebmuh]

No thought at all from my side (not being a coder here). Best chances for you @pvint to get some cleverness is to get @mauser and/or @jeremyz here I'd guess.

The only thing coming up to my mind is: why would this behavior happening on your side (and on @hypsinom 's one), but not on mine? Qt version? This is Qt5 version 5.7.1 here on Debian stretch (which is currently Debian stable).

Hope that helps.

[pvint]

Thanks Olivier
I'm leaning towards QT version too - mine's 5.9.2 and I just tested on another system with a lower version and its OK.

Still working on it :)

[trebmuh]

@hypsinom @SpotlightKid : could you please tell your Qt version?

[pvint]

Well, after much more digging I find this:

When an item in an LCDCombo menu is selected the changeText() method is called:
connect( pop, SIGNAL( triggered(QAction*) ), this, SLOT( changeText(QAction*) ) );

In the changeText() method is called it gets the text from the selection in the menu:

void LCDCombo::changeText(QAction* pAction)
{
        set_text( pAction->text() );
}

It is here that I first see the "&" prepended to the text. This occurs on all LCDCombo menus, for example when I switch from Velocity to Pan the text is retrieved as &Pan.

I have fixed this locally by changing the changeText method to:

void LCDCombo::changeText(QAction* pAction)
{
        QString text = pAction->text();
        // QT 5.9 is returning values with "&" prepended
        // Delete it if it is there
        if (text.at( 0 ) == '&' )
        {
                text.remove( 0, 1 );
        }
        set_text( text );
}

This eliminates the mysterious '&' and works fine, but until it is understood better I'm not sure I'm comfortable calling it a "fix"

[SpotlightKid]

could you please tell your Qt version?

5.10.1

[jeremyz]

Hi,
I'm on archlinux Qt 5.10.1-1
I can't reproduce it, but as I undersand the thinggy,
It looks like an ampersand for keyboard shortcuts is added by Qt when the menu is built.

2 things that could be interesting to check.

• can you switch from Velocity to Pan, hitting P/p V/v while the drop down menu is open ?
• what happens in LCDCombo::update() with the following ???

    for( int i = 0; i < items.size(); i++ ) {
            if ( items.at(i) != SEPARATOR ){
                    INFOLOG(items.at(i));
                    QAction *a = pop->addAction( items.at(i) );
                    INFOLOG(a->text());
            }else{
                    pop->addSeparator();
            }

[trebmuh]

can you switch from Velocity to Pan, hitting P/p V/v while the drop down menu is open ?

I can't here.

[jeremyz]

I made some house keeping in LCDCombo,
can someone who can reproduce this issue,
add the following WARNINGLOG in LCDCombo::addItem

actions.append( pop->addAction( text ) );
WARNINGLOG(QString( "'%1' -> '%2'" ).arg( text ).arg( actions.last()->text() ) );
return true;

then run with -V0x7 and report here please ?

[pvint]

With that line there we do not see the '&':

(I) LCDCombo::LCDCombo INIT
(W) LCDCombo::addItem '4' -> '4'
(W) LCDCombo::addItem '8' -> '8'

(Edit: To be clear: problem still exists.)

No more time for tonight - I will try to have a look tomorrow night.

[jeremyz]

I compiled Qt-5.9.2 base && xmlpatterns from http://download.qt.io/official_releases/qt/5.9/5.9.2/submodules/
linked h2 to it, was thrilled to see that issue ... none !!!

[jeremyz]

If really needed, I suggest this :

set_text( (text.at( 0 ) != '&' ) ? text : text.mid(1) );

[pvint]

linked h2 to it, was thrilled to see that issue ... none !!!

@jeremyz I'm not 100% certain that I know if that means you duplicated the issue or not ;)

As for the "fix": I love the ternary but it has a habit of making things less readable, so I like my longer way using the "if" a bit better. Doesn't really matter one way or the other though ;)

I'm tempted to put the "quick fix" in, but I would really like to understand it better first.
I'd say that if this is blocking many people let's go ahead and put the patch in and leave this issue open to try to sort it out for the long-term.

[jeremyz]

@pvint I haven't been able to duplicate it :/
I really wanted to see it, because it doesn't mean anything.
I even spend some time looking in Qt's change logs and git logs, I saw nothing !!!

That's all there is in our side :

 QAction* = QMenu.addAction(const QString &text)
 changeText(QAction*) -> display->setText( action.text() );

If I couldn't replicate that bug with Qt 5.9.2, maybe it's something within harfbuzz ??

I say push your fix in !!!

[jeremyz]

@pvint
That QString based LCDCombo API is gone.
Good bye ugly !!
This issue should then be fixed … ?

[pvint]

@jeremyz YES!!! It works!
Great idea - not only fixed the problem but is an improvement on how it was done before. Great work!

[trebmuh]

@jeremyz and @pvint : congratz guys!

@hypsinom : can you retry from a fresh git build to check if this is fixed for you as well?