Create common module for fretted string instruments

[jgarte]

This is an open issue to consolidate code in order to avoid duplication when adding more string instruments to chord-editor in the future.

@newlandsvalley What do you suggest we consolidate?

[newlandsvalley]

My recommendation would be initially just to consolidate Guitar and TenorGuitar. Ideally we would be able to extract enough common stuff into (say) FrettedInstrument so that just about all that is left in these is a configuration file for the particular instrument we want. If this were possible we could implement Uke simply by configuring it.

Then we could take a look at Bass to consider whether or not it is worth the effort to do the same with that - it may not be.

As for directory structure I would recommend putting FrettedInstrument at the same level as Piano and Bass and placing the other (now three) beneath it.

I'd like to attempt this work under a branch named (say) Restructure.

[newlandsvalley]

I've created a branch restructure for the consolidation work.

[jgarte]

I've created a branch restructure for the consolidation work.

Thanks letting me know. I'll take a look! :)

[newlandsvalley]

It's just a copy of master at the moment. How do you want to take this forward, @jgarte ? Do you want to have a crack at it yourself, or should I make a start on it? I'd be happy either way,

[jgarte]

It's just a copy of master at the moment. How do you want to take this forward, @jgarte ? Do you want to have a crack at it yourself, or should I make a start on it? I'd be happy either way,

Hi @newlandsvalley, I saw that now. Yes. I'd like to take a crack at it myself if you don't mind waiting till latest, next Monday (7 days). I think it would be a good refactor job (lower hanging fruit) for me to learn from if you don't mind waiting for me to submit the PR. It would be much appreciated :) I'll be starting work tomorrow and won't be able to hack on this in depth till this coming Saturday. It's possible that I might be able to open the PR sooner than Monday but I can't promise it currently.

So far I am thinking to make the following changes:

1. I'll turn stringCount into a function that takes one argument instead of a constant used in two places like it currently exists:

a. https://github.com/newlandsvalley/chord-editor/blob/master/src/TenorGuitar/Graphics.purs#L71 b. https://github.com/newlandsvalley/chord-editor/blob/master/src/Guitar/Graphics.purs#L71

Or what do you think of using records to hold constants like that instead of a function like I described in 1.?

2. This also will need to be a one argument function: https://github.com/newlandsvalley/chord-editor/blob/master/src/TenorGuitar/Types.purs#L42

3. And this one too: https://github.com/newlandsvalley/chord-editor/blob/master/src/TenorGuitar/Types.purs#L59

4. And this one: https://github.com/newlandsvalley/chord-editor/blob/master/src/TenorGuitar/Types.purs#L54

5. This >= 4 needs to be modular on 6 or 4: https://github.com/newlandsvalley/chord-editor/blob/master/src/TenorGuitar/Validation.purs#L53

The above list is not comprehensive but I'm just sharing some of the places I'm thinking of consolidating so that the tenor guitar and guitar can share that code.

[newlandsvalley]

I would be delighted if you take a look at it, @jgarte and please take as long over it as you want. The impetus for tenor guitar and uke comes from you, anyway, and I'd be very happy eventually to have them both supported, but with common data factored out.

If I were you, I'd start with a data structure defined in the FrettedInstrument module and gather together all the primary constants that are needed. If they are truly invariant across all fretted instruments, they can be left as they are as constants. Where they vary, then I'd gather them into a data structure if they are primary (not calculated). So you may end up with something like:

type FrettedInstrumentConfig = 
  { name :: String 
  , stringCount :: Int 
  , ......
  , instrumentIndex :: Int -- index into the array of SoundFont instruments (if these vary)
  , .....
  }

I hope that the calculated constants such as neckWidth or nutDepth will not vary across instruments. If so, leave them as they are. If not - I'm not sure yet what's best to do.

And then pass this data structure around to any function that requires any of these variable configurations. You have two options for doing this - either simply by adding a parameter to each function, or else, you could use the Reader Monad to look them up. Whether the Monad approach is necessary rather depends on how messy the code looks if you're passing the config down cascades of functions or not.

As far as the top level Halogen component is concerned, it is possible to parameterise the StringInstrument component with the Config which can then be incorporated into the overall State for that component.

[jgarte]

Hi @newlandsvalley, I've been so busy with work. I think I won't be able to try again till this weekend. That said, I started working on it and I realized that this will take me a lot longer than what I initially thought it would. There's still a lot of fundamentals that I don't understand about PureScript that I have to iron out.

Feel free to get started on this refactor.

The other option is if you'd like to pair on it I'd be happy to meet up over jitsi this weekend to work on it together. But no pressure if you prefer to work on it by yourself.

[newlandsvalley]

Hi, @jgarte. I quite understand - to be working at a proper job and then to put the hours into a project like this can be very demanding. Much as I'd love to pair with you, I'm afraid this weekend is impossible. I think I'll take an initial look at the refactor myself if you don't mind. I should have a little time today.

[jgarte]

Sounds good! Keep me posted :)

[newlandsvalley]

I've pushed to the restructure branch and got it basically working. Done a bit of testing - only fault so far that I've found is that refresh on the tenor guitar page fails.

The FrettedInstrument module is pretty much as outlined above. The tricky bits are:

• Adding Input to the Page
• Amending the routing so that for example /guitar becomes /frettedInstrument/guitar This involves a good deal of boilerplate in Types which goes between the instrument type and a string. There's still a bug somewhere here I think - probably to do with capitalisation. I've retained the old Guitar and TenorGuitar modules for reference, but they're not called. When we get rid of them, we need also to get rid of their read/write functions in the JSON serializer.

I don't think passing around FrettedInstrumentConfig everywhere is too much of a problem except for in the Graphics module where we may feel we like to neaten things up using the Reader monad so that we don't have it explicitly passed to each and every function.

[jgarte]

I'm curious to see how you end up using the Reader monad. I'll give restructuring and your latest commit a reading to learn from it. Thanks!

[newlandsvalley]

OK - I'll maybe do this myself because I think I remember seeing you say on Discord that Monads were still a bit of a mystery. I think the next stage is to remove the old Guitar and TenorGuitar modules and then modify the tests which must use FrettedInstrument instead.

Again, very many thanks for giving this so much attention.

[newlandsvalley]

Have a look at the Graphics module in the reader branch. I'm not totally convinced that this is an improvement.

• I think the code is longer and harder to understand
• canvasWidth is not looking good because it is called from Page with an explicit config but has to use the Reader because it calls neckWidth which is embedded in a Reader.
• I couldn't find a way to convert finger to a monadic context because I think I'd need something like MapMWithIndex and couldn't find one. So it uses an explicit context but fingering which calls it is monadic.
• You may see the benefit in buildChord but I'm not sure it is worth the trouble.

What do you think, @jgarte ?

[jgarte]

I think I prefer the version without the reader monad as it seems easier for me to understand ;()

But don't let that hold you back if you prefer the reader monad version. It sounds like you may not though.

[newlandsvalley]

I agree entirely - it's not bought us much and had added to the complexity. I'll remove the reader branch.

[newlandsvalley]

I've made the changes you recommended, @jgarte, when you reviewed the restructure and so I've now merged them into master and have deleted the restructure branch. Very many thanks.

[jgarte]

@newlandsvalley Thank you for the collaborations. It was fun and I hope for more to come :)