rationalize definition of tuning presets

[guiv42]

note: this issue is precisely documented with the intention to make it accessible to a newcomer.

description of issue

A list of 5 tuning presets is hard-coded in MidiSongReader: it is used to select an appropriate tuning preset when importing a midi file. This list has been there for a while.

More recently, a configurable list of tuning presets was added in the form of a tunings.xml file. So now, some identical tuning presets are coded at 2 different places.

Objective is to cleanup code: remove duplicated definitions to ease future maintenance. In a first step, actual behavior should remain unchanged. Then, tuning preset selection when importing midi files could be improved just by editing the xml.

how to fix

Proposal: make the priority of tuning presets configurable in xml file. This is the way I would implement it:

• add an optional attribute priority to tuning presets in tuning.xml file. Define value 1 for standard E-tuning, 2 for Dropped-D, etc, to reproduce priorities currently hard-coded in MidiSongReader.
• Add an integer attribute priority in TGTuning class, with value imported from xml file content (and a default value if no attribute in xml). Import of tuning attributes from xml is done in TuningReader
• implement Comparable<TGTuning> interface in TGTuning class, so instances can be sorted by priority order (instances with no priority attribute should come last)
• in TuningManager, add a method getPrioritizedTgTunings, which returns all TG tuning presets with a defined priority attribute, sorted by its value (refer to getTgTunings). Discard tuning presets with no priority.
• in MidiSongReader (TrackTuningHelper.getStrings), delete if () else if() sequence over successive hard-coded presets. Retrieve list of presets sorted by priority from TuningManager. Then, loop over this list until an acceptable tuning preset is found (min/max criterion unchanged)
• if none is found, keep actual strategy to create ad hoc tuning (the last else condition)

how to test

See attached files below. testTuning.zip

With menu "File/Import/Import Midi", import midi file from archive (transpose 0). Result should be identical to the .tg file also included in the archive. Each track corresponds to one current if case (5 hard-coded tuning presets + the last else condition). When writing this issue this test currently passes (169a5e14371584cacc75fcc9d093152af2bf916c).

Note: that would be much better if this test could be directly implemented in the code base, instead of keeping it manual!

[tdan]

Thanks @guiv42 for the detail steps. I'm working on this issue now :)

[guiv42]

Just for information, another place where "default tunings" are defined: https://github.com/helge17/tuxguitar/blob/6e5bd2adb15cfa6111bff75dcd6d7dc7ed8125ca/common/TuxGuitar-lib/src/org/herac/tuxguitar/song/managers/TGSongManager.java#L25

[tdan]

Just for information, another place where "default tunings" are defined:

https://github.com/helge17/tuxguitar/blob/6e5bd2adb15cfa6111bff75dcd6d7dc7ed8125ca/common/TuxGuitar-lib/src/org/herac/tuxguitar/song/managers/TGSongManager.java#L25

I got the first part down, ready to merge, but not sure how I should go about this part. Should "Default Tunings" be Prioritized Tunnings List from the xml file?

[guiv42]

Let's do it step by step, especially for a first contribution. So, if you have something ready, just create a pull request now.

It would be better if we can remove all duplicated data, but this can be done afterwards.

[guiv42]

Thanks @tdan for the implementation, you PR is now merged.

I think it's worth looking at the hard-coded tuning presets still defined in TGSongManager before closing this issue. The 2 questions to be answered:

1. what are these hard-coded tuning presets used for?
2. is it possible to remove them and re-use those defined in xml file?

@tdan: do you want to take care of this, or do you prefer me to remove you as "assignee"? (no good or bad solution, it's up to you)

[tdan]

I'll be happy to take a look :)

[tdan]

Hi @guiv42,

The hard-coded presets are the default tunings assigned whenever you add a new track to a song.

We can reuse the tunings defined in the XML file by leverage TuningManager. We'll use it to retrieve priorityTgTunings(). Then, in the createStrings() method, we'll find the highest priority tuning (lowest numerical value) that is associated with the number of strings requested.

The challenge lies in obtaining a TGContext object. Currently, the only approach I can see involves retrieving a TuxGuitar instance and extracting the context from it. Here's an example of how it might look:

public List<TGString> createDefaultInstrumentStrings(int stringCount) {
    TuxGuitar tgInstance = TuxGuitar.getInstance();
    TuningManager tuningManager = TuningManager.getInstance(tgInstance.getContext());

    List<TGTuning> defaultTunings = tuningManager.getPriorityTgTunings();

    return createStrings(stringCount, defaultTunings);
}

However, this approach requires adding org.herac.tuxguitar to tuxguitar-lib as dependency. I'm not sure if this is the most suitable solution, given my limited experience.

[guiv42]

Thanks for your clear analysis.

this approach requires adding org.herac.tuxguitar to tuxguitar-lib as dependency

You cannot do that, for 2 reasons:

• there is already a dependency from tuxguitar to tuxguitar-lib, so that would create a circular dependency
• you shall not include any dependency from any module in common folder to a module out of this folder. All modules in "common" folder are used by both desktop and android apps, so for example adding a dependency to a module in "desktop" folder would break Android app.

So I suggest leaving TGSongManager unchanged, I think it's not worth a deep rework.

However this made me realize that we have a problem in the Android app, where the tuning.xml file was duplicated. Now it's missing the priority attribute. This problem illustrates the risk of data duplication: inconsistency. Logically PR #319 should have broken the "midi import" feature of Android app. Practically it seems it did not, because feature was already broken before! (don't know why).

So, I propose to

1. close this issue
2. replace tuning.xml file in Android resources by a symlink to tuning.xml file of the desktop app
3. create a new issue for "midi file import" feature in Android app

If that is OK for you, just close this issue I can take care of 2. and 3.

Edit: status of "midi file import" feature in Android app is not so clear. I have different behaviors on different devices (virtual device / real device). At least on one of them PR #319 did break the feature. Action 2. mentioned above is clearly needed.

[guiv42]

Action 2. has been done (#371). Interesting info: this action has shown that previous fixes implemented in "tuning.xml" file for desktop app were forgotten in Android app. So, this issue is perfectly relevant: avoid duplicating data!

By the way, #371 also extends #319 to Android, so midi import now works correctly (action 3. mentioned above is no more needed)

@tdan I think this issue can be closed, it's up to you

[tdan]

Sounds good to me. I'm glad the both desktop and Android version are working correctly with the tuning.xml file

I'll start working on #289 like I mentioned in that thread, unless you want me to take care of something else.

On Wed, May 1, 2024, 10:55 guiv42 @.***> wrote:

Action 2. has been done (#371 https://github.com/helge17/tuxguitar/pull/371). Interesting info: this action has shown that previous fixes implemented in "tuning.xml" file for desktop app were forgotten in Android app. So, this issue is perfectly relevant: avoid duplicating data!

By the way, #371 https://github.com/helge17/tuxguitar/pull/371 also extends #319 https://github.com/helge17/tuxguitar/pull/319 to Android, so midi import now works correctly.

@tdan https://github.com/tdan I think this issue can be closed, it's up to you

— Reply to this email directly, view it on GitHub https://github.com/helge17/tuxguitar/issues/269#issuecomment-2088585233, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABEDDAQPEBMZJYS7TJKDLCLZAD66NAVCNFSM6AAAAABEOJP5MSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBYGU4DKMRTGM . You are receiving this because you were mentioned.Message ID: @.***>

[guiv42]

OK, so I close this issue. (and you can work on whatever you want)