Move to PyQt5

[Whatang]

PyQt4 is very old. When moving to Python3 in #413 , also look at converting DB to PyQt5.

[InTheWorks-ca]

@Whatang Python3 no longer has support for PyQt4 so they must be done together. I'd say close #413 and update this issue.

I've started working on this. It's on a branch.

Using the 2to3 script the python2 to python3 went mostly well. I was able to fix all the problems though I'm not confident those fixes are correct. Moving to PyQt5 did not go so well.

After fixing the imports for PyQt5 and commenting out some code, I was able to get DrumBurp to open. There are a few things that are horribly broken (like tab editing) and I'm not sure how to go about fixing them.

For PyQt5, the biggest pain is probably that setHandlesChildEvents() has been removed. Event handling is different. Slots changed, I'm not sure about the signalling.

I am not an expert in python or PyQt5. I do have some experience with Qt5 which is now getting kinda old. Should probably migrate to Qt6 as soon as Qt5 is working.

Some of the version numbers are embedded in .ui files, but I don't think I need to run the ui builder to regenerate any files. I just edited version numbers only so they show up in git and can be set properly later.

Not surprising, but

pygame version==1.9.1

Doesn't exist for python3. The lowest version is 1.9.2 and if I try to install it, there is an error about a missing freetype-config. Apparently that was removed from Ubuntu 20.04. No workaround I could see. The last 1.9.x version was 1.9.6 and this didn't install either, so I just installed the latest (without issue).

Python versions:

python 3.8.10
pip 24.2 (needed to install PyQt5)
PyQt5 5.15.14
pygame 2.6.0

I am on Ubuntu 20.04 and using venv. Here's how I'm developing:

git clone git@github.com:InTheWorks-ca/DrumBurp.git
python3 -m venv venv
. venv/bin/activate
cd DrumBurp
git checkout python3-and-qt5
pip3 install --upgrade pip
pip3 install pyqt5 --config-settings --confirm-license= --verbose
pip3 install pygame
python3 src/DrumBurp.py

Midi isn't working. When I run DrumBurp as above, I see the following on the terminal:

pygame 2.6.0 (SDL 2.28.4, Python 3.8.10)
Hello from the pygame community. https://www.pygame.org/contribute.html
libpng warning: iCCP: known incorrect sRGB profile
QWidget::setTabOrder: 'first' and 'second' must be in the same window
QGraphicsScene::addItem: item has already been added to this scene
QGraphicsScene::addItem: item has already been added to this scene
...
ALSA lib conf.c:4555:(snd_config_update_r) Cannot access file /usr/local/share/alsa/alsa.conf
ALSA lib seq.c:935:(snd_seq_open_noupdate) Unknown SEQ default
QGraphicsScene::addItem: item has already been added to this scene
QGraphicsScene::addItem: item has already been added to this scene
...
QGraphicsScene::addItem: item has already been added to this scene

The QGraphicsScene::addItem warning is very noisy.

/usr/local/share/alsa/asla.conf doesn't exist on my machine, but /usr/share/alsa/alsa.conf does. So I made some symlinks:

sudo ln -s /usr/share/alsa /usr/local/share/alsa
sudo ln -s /usr/lib/x86_64-linux-gnu/alsa-lib /usr/local/lib/alsa-lib

That got rid of alsa warnings, but created a new problem that I fixed. That enabled the midi menu, but play broke it. It seems like the data types arriving at DrumBurp aren't what's expected (float vs int). If I patch that, it fails at

            pygame.mixer.music.load(midi)

with error:

    pygame.mixer.music.load(midi)
pygame.error: Couldn't read first 12 bytes of audio data
Aborted (core dumped)

I think the semantics of pygame.mixer.music.load() have changed. Probably now is a good time to move to mido.

[InTheWorks-ca]

I was able to fix the midi. I can play and stop and play the score without issue. I had to change StringIO to BytesIO in a couple of places. And then coerce those string values in exportMidi to be bytes. It now looks like this:

def exportMidi(measureIterator, score, handle):
    handle.write(b"MThd\x00\x00\x00\x06\x00\x00\x00\x01")
    handle.write(((MIDITICKSPERBEAT >> 8) & 0xFF).to_bytes(1, byteorder='big'))
    handle.write(((MIDITICKSPERBEAT >> 0) & 0xFF).to_bytes(1, byteorder='big'))
    notes, baseTime = _calculateMidiTimes(measureIterator, score)
    midiData = _makeMidiStart(score)
    midiData += _writeMidiNotes(notes, baseTime)
    midiData = _finishMidiData(midiData)
    handle.write(b"MTrk")
    for byte in midiData:
        handle.write(int(byte).to_bytes(1, byteorder='big'))

And in _playMIDINow, it looks like this:

            midi = io.BytesIO()
            exportMidi(measureList, score, midi)
            midi.seek(0, 0)
            pygame.mixer.music.load(midi) # after this midi is closed
            pygame.mixer.music.play()

I had to clean up opening and closing pygame.midi. I took out a bunch of functions related to setting the port because it can't be done with pygame anyway. When pygame.midi.Output() was called more than once, even after quitting the pygame modules, there was an error that was inside pygame that I didn't want to locate and fix. Refactoring the code as I did prevents that error from occurring. At least on linux.

Midi playback works now, at least after the alsa symlinks were added. I don't like using pygame for this and I have been playing with pyfluidsynth. I may attempt to integrate that later along with mido for output to any midi port.

From what I can tell, export to midi and export to ascii both work.

However, there are two serious problems at the moment:

1. The score cannot be edited. There are no warnings or errors when clicking in the score area, but something's not hooked up anymore. The mousePressEvent() function gets called in QScore, but I don't know what's supposed to happen next.

2. Kit editing doesn't work. Double-click on the drum key causes an error:

Traceback (most recent call last):
  File "/path/to/DrumBurp.git/src/GUI/QKitData.py", line 43, in mouseDoubleClickEvent
    self.scene().editKit()
  File "/path/to/DrumBurp.git/src/GUI/QScore.py", line 937, in editKit
    emptyDrums = set(self.score.drumKit)
TypeError: unhashable type: 'Drum'
Aborted (core dumped)

@Whatang, if you know how to fix that one I would appreciate some help. Python is not my language of choice so I'm just hacking. From what I've seen the Drum class implements iter() and getitem() so it should be iterable, but I don't know what the intent of this code is:

        emptyDrums = set(self.score.drumKit)
        for staffIndex in range(self.score.numStaffs()):
            lines = set(self.score.iterVisibleLines(staffIndex, True))
            emptyDrums.difference_update(lines)
            if not emptyDrums:
                break

There was a third issue which is all the:

QGraphicsScene::addItem: item has already been added to this scene

Briefly, this was because in QLineLabel.py

class QLineLabel(QGraphicsObject):
    def __init__(self, drum, qScore, parent):
        super(QLineLabel, self).__init__(parent=parent)
        if qScore is not None: qScore.addItem(self)

And this function in QStaff.py:

    def _addLineLabel(self, drum):
        qLabel = QLineLabel(drum, self._qScore, self)
        self._lineLabels.append(qLabel)
        self.addToGroup(qLabel)

Was causing a double add. It doesn't appear that QLineLabel is used anywhere other than QStaff, so it was safe to simply remove the qScore.addItem() from QLineLabel's constructor.

QStaff uses addToGroup() for QMeasureLine, QMeasure, and QLineLabel and there is no addItem() in any of their constructors.

At the moment, the Section titles (ie. QLineLabel) can be edited. The Title can be edited. Everything can be shown or hidden.

However, the measures themselves can't be edited and opening the drum key editor dialog causes the whole program to crash. And Lilypond export crashes..

[InTheWorks-ca]

So it was the presence of this function that threw the unhasable type error.

def __eq__(self, other):
    if not isinstance(other, Drum):
        return False
    return self.name == other.name or self.abbr == other.abbr

I commented it out and got different errors to fix. I didn't see it used anywhere.

After more changes, the edit drum key opens, and the file dialogs work.

One of the issues I found was that the python3 / operator will produce float output. Absolutely bonkers for it to change the type. Python is a stupid language and that's a good example of why.

The fix was to find all the / operators and replace them with //. This was easy enough with sed thanks to consistent formatting.

I also got lilypond export to work.

So the only obvious problem is that the score can't be edited.

[InTheWorks-ca]

It's awfully quiet in here...

I'm still plugging away at this. I don't know why events aren't propagating, but I decided to turn my attention to the unit tests. I don't know how they are supposed to be run, but cd src/test and python3 theTest.py has errors finding relative paths. I don't know the correct solution, but adding

import sys
sys.path.append("..")

Before any includes seems to work well for the files under src/test.

I ran into a problem that is rather bothersome and I don't know if it affects any other part of the program. The issue is that sets are not ordered. In testdbfsv0.py there is an expectation that if a Notehead is added without a specified shortcut, it should come back with a.

        self.assertEqual(outlines,
                         ["KIT_START",
                          "  DRUM One,d1,x,True",
                          "    NOTEHEAD x 71,96,normal,default,0,none,0,x",
                          "    NOTEHEAD g 71,96,ghost,default,0,ghost,0,g",
                          "  DRUM Two,d2,o,False",
                          "    NOTEHEAD o 71,96,normal,default,-5,none,1,o",
                          "    NOTEHEAD O 71,96,accent,default,-5,accent,1,a",
                          "KIT_END"])

When the test runs, the shortcut is not a. It's a letter, but not always the same. Sets are not ordered so I guess that should be expected. But python2 and python3 produce different sets:

Python 2.7.18 (default, Jul  1 2022, 12:27:04)
>>> availableShortcuts = set('abcdefghijklmnopqrstuvwxyz')
>>> print availableShortcuts
set(['a', 'c', 'b', 'e', 'd', 'g', 'f', 'i', 'h', 'k', 'j', 'm', 'l', 'o', 'n', 'q', 'p', 's', 'r', 'u', 't', 'w', 'v', 'y', 'x', 'z'])

Python 3.8.10 (default, Nov 14 2022, 12:59:47) 
>>> availableShortcuts = set('abcdefghijklmnopqrstuvwxyz')
>>> print(availableShortcuts)
{'k', 'w', 'r', 't', 'f', 'b', 'c', 'q', 's', 'p', 'i', 'x', 'y', 'j', 'h', 'l', 'n', 'v', 'u', 'e', 'g', 'm', 'a', 'z', 'd', 'o'}
>>> 

A simple solution is to just sort the set in this case:

>>> availableShortcuts = sorted(set('abcdefghijklmnopqrstuvwxyz'))
>>> print(availableShortcuts)
['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z']

The code would actually need:

availableShortcuts = sorted(set('abcdefghijklmnopqrstuvwxyz'), reverse=True)

so that availableShortcuts.pop() returns a as expected.

But the bigger issue is if elsewhere in the code, the sets are expected to be sorted. I don't know so for now I will make that one change to fix that test case.

And there is another issue regarding ordered elements. In DBConstants.py we have:

BAR_TYPES = {NO_BAR: 0,
             NORMAL_BAR: 1,
             REPEAT_START: 2,
             REPEAT_END_STR: 4,
             SECTION_END: 8,
             LINE_BREAK: 16}

And in python2 it looks like:

>>> print BAR_TYPES
{'NORMAL_BAR': 1, 'REPEAT_START': 2, 'LINE_BREAK': 16, 'NO_BAR': 0, 'SECTION_END': 8, 'REPEAT_END': 4}

Whereas in python3 it looks like:

>>> print(BAR_TYPES)
{'NO_BAR': 0, 'NORMAL_BAR': 1, 'REPEAT_START': 2, 'REPEAT_END': 4, 'SECTION_END': 8, 'LINE_BREAK': 16}

So python2 isn't ordered, but python3 is ordered. To make it work, I have to make the order as per python2.

# python3 compatibility, maintain this order.
BAR_TYPES = {NORMAL_BAR: 1,
             REPEAT_START: 2,
             LINE_BREAK: 16,
             NO_BAR: 0,
             SECTION_END: 8,
             REPEAT_END_STR: 4}

I have no idea if this is right, but testdbfsv0.py passes now.

Of course, the next problem is utf8 handling between python2 and python3 in testdbfsv1.py.

class Base64StringField(SimpleValueField):
    def _processData(self, data):
        try:
            data = data.decode("base64").decode("utf8")
        except binascii.Error:
            raise DBErrors.BadBase64()
        except UnicodeError:
            raise DBErrors.BadUnicode()
        return data

    def _toString(self, value):
        return value.encode('utf8').encode('base64').strip()

In other parts of the code, it was easy enough to prepend b to strings (ie. 'my string' becomes b'my string') and that fixed most everything. However, this error is in fileUtils.py. I don't know what's an appropriate way to convert it. As of right now the tests that are failing for me are:

testdbfsv1.py
testLilypond.py
testMeasureCount.py
testNotePosition.py
testScore.py (fails because of NotePosition)
testScoreSerializer.py (fails because of the same problems in fileUtils.py)

NotePosition is failing because __cmp__ has been done away with in Python3. Must define __lt__ and friends. But that should be easy to fix, unlike that utf8 handling.

[InTheWorks-ca]

testMeasureCount.py passes now. For some reason it was missing a positional argument called swing. So I just added False to those function calls and everything is happy.

testNotePosition.py passes now after adding __eq__, __lt__, __le__, __gt__, and __ge__ methods to NotePosition.py.

testScore.py passes, but I had to do some string manipulation. In Score.py I changed the output of hashScore() to return the hex encoded version since the only place hashScore() was used did .encode('hex') which no longer works in python2

    def hashScore(self):
        exporter = AsciiExport.Exporter(
            self, ASCIISettings.ASCIISettings(), False)
        scoreString = StringIO()
        exporter.export(scoreString)
        scoreString = scoreString.getvalue()
        hashme = hashlib.md5(scoreString.encode('utf-8')).digest()
        return codecs.encode(hashme, 'hex_codec')

testLilypond.py seems to not have been updated for the tuplet code. The change I made looks like this:

         self.assertEqual(output,
-                         r'\new Lyrics \with { alignAboveContext = #"main" } \lyricmode { " "4 " "4 " "4 \times 2/3 { L8 R8 L8 } }')
+                         r'\new Lyrics \with { alignAboveContext = #"main" } \lyricmode { " "4 " "4 " "4 \tuplet 3/2 { L8 R8 L8 } }')

The rest of the fixes involve some manner of utf-8 handling.

The unit tests now run without errors. And I've pushed the fixes to my branch and it would be great if someone could take a look.

@Whatang hasn't been active on github for some time. I hope he's well.

@Brayconn I saw you did some excellent work improving Lillypond output. Might I ask you to please have a look at the python3 changes on my branch and point out anything that looks stupid?

Fixes for the unit tests did not fix the major problem that the Score can't be edited. I'll continue digging.

[Brayconn]

Oh wow, didn't realize anyone was working on this

Might I ask you to please have a look at the python3 changes on my branch and point out anything that looks stupid?

I can try taking a look, though

1. I've never used QT before, so I'm not sure how much I'll be able to help with that part specifically
2. I've been super busy with university as of late, so I can't guarantee that my code review will be fast

With that said...

I saw you did some excellent work improving Lillypond output.

I'm 99% sure that my contribution actually has an off-by-one error in the code that determines note length. I've encountered many times where the output just breaks when doing weird stuff with quintuplets, septuplets, or even 32nd triplets, and the cause has almost always been that it generated quarters instead of 8ths, or 8ths instead of 16ths, etc. tbh, I only realized after I made my last PR that there even were tests, and it wasn't until way later that I got into the habit of adding tests on my own code, so...

Semi-related: I realized way later that the way the current code generates sticking is almost certainly wrong. The current method causes dense sticking to go way above/below where its supposed be which causes a huge mess on the page. It's been a while since I looked into the solution, but I think it should be using the method shown here under "Drum rolls" (the ^ and _ notation).

So yeah, I think the testing for the lilypond code in general needs a lot more work in general. I can provide some scores that cause these issues if needed.

[InTheWorks-ca]

Oh wow, didn't realize anyone was working on this

There was a pull request migrating to python3 that never got integrated and now there's no qt4 available for python3. There's some improvements I'd like to make, namely to the lilypond export, that I can't do on a modern system. So I figured I'd roll up my sleeves and try updating DrumBurp. How hard could it be? ;p

Might I ask you to please have a look at the python3 changes on my branch and point out anything that looks stupid?

I can try taking a look, though

1. I've never used QT before, so I'm not sure how much I'll be able to help with that part specifically

Fair enough. I've used Qt before as qml or c++. Never python. And python being a scripted language adds a lot of difficulty (no compile time checks). It doesn't help that my day job is coding in c and I don't understand the more pythonic ways of writing python code.

2. I've been super busy with university as of late, so I can't guarantee that my code review will be fast

I'd appreciate whatever time you can spare. This isn't going very quickly anyway.

I'm 99% sure that my contribution actually has an off-by-one error in the code that determines note length.

That would be good to investigate after DrumBurp is working on python3 and Qt5. The change in meaning for / between python2 and python3 caused some grief and I'm not certain I've got all the divisions in the right places. Integer division uses // now, but because there's no strict typing it's hard to tell without actually running that code and encountering an error. So I did a find and replace on files changing all '/' to '//'. I had to revert one back to '/' when I ran into a problem so there may be others wherever that off-by-one error happens to be ;)

I don't use DrumBurp at the moment for lilypond transcription because the stem directions in the output don't work for me. I should be able to change that in the kit definitions, but there seems to be issues loading and saving kits.

Semi-related: I realized way later that the way the current code generates sticking is almost certainly wrong. The current method causes dense sticking to go way above/below where its supposed be which causes a huge mess on the page. It's been a while since I looked into the solution, but I think it should be using the method shown here under "Drum rolls" (the ^ and _ notation).

Good to know, I haven't used sticking in any notations yet.

So yeah, I think the testing for the lilypond code in general needs a lot more work in general. I can provide some scores that cause these issues if needed.

Perhaps once the major issues have been dealt with and a python3/Qt5 release can be built.