search

music-encoding / sibmei

Sibelius MEI Plugin
MIT License
39 stars 16 forks source link

Libmei SetId() and RemoveChild() fix #178

Closed th-we closed 3 years ago

th-we commented 3 years ago

... to not break getElementById() or getElementsByName()

ahankinson commented 3 years ago

removeKeyFromDictionary does what it says -- it removes the key from the dictionary. It's not particularly graceful, but few things in ManuScript are.

I would expect that the behaviour would differ when calling PropertyExists on a dictionary with the ID, but an empty value?

th-we commented 3 years ago

As MEIFlattened becomes quite large, I felt it was not a good idea to re-create it every time SetId() is used. But at the moment, Sibmei only uses SetId() in the header and it's therefore not an issue. If it was used in the body, that might mean O(n²) performance.

I also realize that one change required for this slipped into the wrong commit.

https://github.com/music-encoding/sibmei/pull/173/commits/e9c576982152ceef8c93fe50db96451b250c86c0#diff-8f908fa55702e2b03b5c2472511fd19030738d0a0f501b61089457bfb87f6354R1887

I can either include that missed line, or rework the pull request to use the old approach of re-generating the dictionary.

ahankinson commented 3 years ago

There's something just a bit icky about keeping IDs around that are 'deleted' -- I would probably prioritise consistency and correctness over speed. What do you think?

(Of course in a 'real' language we wouldn't have these problems, but...)

th-we commented 3 years ago

SetId() isn't so much of a problem, but RemoveChild() might be because we do use it in the body.

At the moment that will not make an impact as the string concatenation happening in convertDictToXml is the bottleneck for larger files. But that can be changed easily.

ahankinson commented 3 years ago

But only in the tuplet handling code, right? That's probably unlikely to have a huge speed impact.

(You're a brave man, sticking your fingers in the tuplet code. Thar be dragons. )

th-we commented 3 years ago

I don't deserve the credit, you wrangled the dragons!

https://github.com/music-encoding/sibmei/blame/develop/src/ExportProcessors.mss#L168-L183

I'll modify the pull request and restore the original behavior to clone the Dictionary.

th-we commented 3 years ago

@ahankinson Any objections against merging the version with removeKeyFromDictionary() reverted to its old behavior? The removed line in the tuplet code is because I moved that line to RemoveChild(). RemoveChild() should do all the necessary bookkeeping to disassociate parent and child.