Variant drawing styles

[dariogf]

There are two new options to review, the strokecolor property that can be used to change only the stroke color in a similar way to the color property, and the context_hook that pass a complete drawing context that can be used to customize the drawing at any level.

[jrobinso]

Thanks this is an interesting PR, and could potentially be generalized for other track types. I won't be able to concentrate on this until next week, busy with some IGV desktop work at the moment.

[jrobinso]

In the meantime, it looks like I also made some variantTrack changes, if you could resolve the conflicts that would be helpful. Not essential though.

[dariogf]

That's great Jim. Will try to find time to solve the conflicts and update the PR. El mié., 4 may. 2022 18:08, Jim Robinson ***@***.***> escribió:

…

In the meantime, it looks like I also made some variantTrack changes, if you could resolve the conflicts that would be helpful. Not essential though. — Reply to this email directly, view it on GitHub <#1502 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFXSOK2HUWZS7JW7AXEDHDVIKOGLANCNFSM5VBCJMWQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[jrobinso]

OK if it takes much time don't worry about it I can do it while reviewing.

[dariogf]

I have resolved the conflicts, and separated the example to a new file.

[jrobinso]

I'll be looking at this this week.

[jrobinso]

@dariogf I had an initial look, looks great, I am confused about 1 point. The "context_hook" is used to customize the drawing of the variant, however the default drawing (a rectangle) is drawn first whether a "context_hook" is defined or not. In your draw implementations you sometimes clear this previously drawn rect, however I think logic like the following would be clearer and more flexible

if(this._context_hook) {
   ... draw with the provided function
} else {
   ... do the default draw. (fill rectangle)
}

Another minor comment, I think we might rename "context_hook" to something more explicit, it wasn't clear to me until looking at the code what it actually does. However I don't have a suggestion at this point, just noting it. Also, I think I will extend this to allow a hook for genotype drawing as well, however I can do that post merge.

[dariogf]

Hi Jim. I agree with your suggestions, both of them. Feel free to make whatever you need. It would be great to skip the default drawing if not necessary, it will also speedup a bit. Now that I am thinking about it, using the default drawing if you return false in the context_hook function would be useful when you only need to override drawings for some variants. That is because I didn't clear the previously drawn context at some times. The name context_hook itself is not very descriptive. Maybe custom_drawing, drawing_override, or something similar should be better. Just some ideas of names also valid for genotypes. El mié., 18 may. 2022 20:35, Jim Robinson ***@***.***> escribió:

…

@dariogf <https://github.com/dariogf> I had an initial look, looks great, I am confused about 1 point. The "context_hook" is used to customize the drawing of the variant, however the default drawing (a rectangle) is drawn first whether a "context_hook" is defined or not. In your draw implementations you sometimes clear this previously drawn rect, however I think logic like the following would be clearer and more flexible if(this._context_hook) { ... draw with the provided function } else { ... do the default draw. (fill rectangle) } Another minor comment, I think we might rename "context_hook" to something more explicit, it wasn't clear to me until looking at the code what it actually does. However I don't have a suggestion at this point, just noting it. Also, I think I will extend this to allow a hook for genotype drawing as well, however I can do that post merge. — Reply to this email directly, view it on GitHub <#1502 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFXSOOJJVWNCJNLAM7GSCDVKUZ63ANCNFSM5VBCJMWQ> . You are receiving this because you were mentioned.Message ID: <igvteam/igv .***@***.***>

[jrobinso]

WRT optionally doing the default draw, I think I'll implement that if function explicitly returns false, in other words no return value (=> undefined) will be converted to true, because I think the general expectation would be the custom draw overrides the default one.

[dariogf]

That sounds fine. El jue., 19 may. 2022 0:53, Jim Robinson ***@***.***> escribió:

…

WRT optionally doing the default draw, I think I'll implement that if function explicitly returns false, in other words no return value (=> undefined) will be converted to true, because I think the general expectation would be the custom draw overrides the default one. — Reply to this email directly, view it on GitHub <#1502 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFXSOLTAMHLTUI77YMGPMTVKVYFVANCNFSM5VBCJMWQ> . You are receiving this because you were mentioned.Message ID: <igvteam/igv .***@***.***>

[jrobinso]

OK I've made the changes discussed, and to my surprise I was able to push them to this branch. If I had thought that would succeed I would have added more comments. Here are the changes:

• strokecolor property renamed to "borderColor", easier to explain
• context_hook renamed to "customDraw"
• If custom draw function explicitly returns false the default variant representation is drawn. I think this only makes sense to use if the customDraw function does nothing, since the default draw will overwrite it.

[dariogf]

Good news, Indeed, I expected it to not be problematic, since the needed changes are not very intrusive. Anyway It will allow great customizations ;) PD.: I have been trying to implement another "cool" feature, but I am afraid that I don't have sufficient knowledge of the project and javascript to achieve it. I will open a new issue to discuss it. El jue, 19 may 2022 a las 21:46, Jim Robinson ***@***.***>) escribió:

…

OK I've made the changes discussed, and to my surprise I was able to push them to this branch. If I had thought that would succeed I would have added more comments. Here are the changes: - strokecolor property renamed to "borderColor", easier to explain - context_hook renamed to "customDraw" - If custom draw function explicitly returns false the default variant representation is drawn. I think this only makes sense to use if the customDraw function does nothing, since the default draw will overwrite it. — Reply to this email directly, view it on GitHub <#1502 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFXSOIOSSM45ESQO7YYY53VK2LC5ANCNFSM5VBCJMWQ> . You are receiving this because you were mentioned.Message ID: <igvteam/igv .***@***.***>

-- un saludo Dario Guerrero ***@***.***