Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove duplicate chords #1216

Merged
merged 5 commits into from
Aug 5, 2023
Merged

Remove duplicate chords #1216

merged 5 commits into from
Aug 5, 2023

Conversation

AdamSEY
Copy link
Contributor

@AdamSEY AdamSEY commented Jul 4, 2023

Issues

Fixes #1209

Proposed changes

Remove duplicate chords

Checklist

  • I consent that this change becomes part of alphaTab under it's current or any future open source license
  • Changes are implemented
  • Existing builds tests pass
  • New tests were added

Further details

  • This is a breaking change
  • This change will require update of the documentation/website

@@ -209,23 +208,24 @@ export abstract class ScoreLayout {
if (notation.isNotationElementVisible(NotationElement.ChordDiagrams)) {
this.chordDiagrams = new ChordDiagramContainerGlyph(0, 0);
this.chordDiagrams.renderer = fakeBarRenderer;
let chords: Map<string, Chord> = new Map<string, Chord>();
let chordNames: Set<string> = new Set<string>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name is not a good indicator for uniqueness. The same chord (by name) might be played differently. That's why in #1209 I indicated that the name might be a good first check but we then need to check further based on the details within the chord.

This might be achieved by pre-computing a unique id in the Chord class based on the information (and we use it here) or we do the check here once and only if needed.

We're creating chord uniqueId based on the chord props.
Generating chord uniqueId based on chord props.
@AdamSEY AdamSEY requested a review from Danielku15 July 5, 2023 14:44
@AdamSEY
Copy link
Contributor Author

AdamSEY commented Jul 5, 2023

I have made the required changes, please check again now the commits and let me know.

Fixing issue with building on Kotlin
Copy link
Contributor Author

@AdamSEY AdamSEY left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought, we don't need to use JSON in chord.ts

@AdamSEY
Copy link
Contributor Author

AdamSEY commented Jul 8, 2023

@Danielku15 Do you have any idea how to fix that, I believe in c# we could use String.Join but not sure why it's not being used in the translation process.

@Danielku15 Danielku15 self-assigned this Aug 5, 2023
@Danielku15
Copy link
Member

@AdamSEY Sorry for the long delay here. I made some modifications and now looks all good. Thanks for the contribution.

In such cases where a new API from JavaScript is used which might not exist in C# or Kotlin, we have three options: try to eliminate the use of the new API and go back to the basics, adjust the transpiler which translates TypeScript to XX to handle the special case or simply add the API in the target language (via extensions or helper methods which are implemented in each platform specifically). The extension is what I've done in this case: Simply adding an extension method to have something like a listOfStrings.Join("Separator") also in C# available. Kotlin has a .join() built in that's why it worked there.

Can be a bit tricky at the beginning.

@Danielku15 Danielku15 merged commit 0f41295 into CoderLine:develop Aug 5, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Equal chords should only be listed once in the chord diagrams.
2 participants