added options to customize token

[Angelinsky7]

i had time... for #20 it's a first version, i certainly forgot some things but the overall plugin is working, and you can customize the token... On my version i had sometime a small issue when saving the other settings tab (not saving) but i'm not an extension dev expert so i'm sure someone could tell me.

[omsharp]

Hey,

Give me sometime to check the code and I'll get back to you.

[Angelinsky7]

I'm not to understand, something like an example file ??

[omsharp]

No. I just need time to look into the code you wrote here.

[Angelinsky7]

ahhh.. sorry misread sometime with something... my bad

[omsharp]

No problem. I'll get back to you as soon as I can, I just need to finish something important on another project.

Thanks for the PR :)

[omsharp]

Hey, I've been looking into the code, good job!

We can definitely make this work, I just have a couple of points and questions to go through.

Points

1 - We need to keep the naming convention consistent throughout the project. 2 - It would be nice to move DataTemplates and Styles into UserControl.Resource just to keep the XAML a bit tidier and easier to work with, for any future changes. 3 - XAML utils should be in the Options folder, since they're specific for XAML and not project wide.

Questions

1 - Do we need all that behavior code? Can't we do with something simpler and less code? 2 - Why not use Dictionary<CommentType, string> for TokenValues, instead of using Dictionary<string, string>? Wouldn't that make things a bit easier?

[Angelinsky7]

Hey, great cool...thanks

Points

1. Yes, totaly agree with you, sorry we need also to keep the same formatting style convention, my bad
2. Ok
3. Yes, but didn't know where to put stuffs

Questions

1. The behaviour code is only there to make things totally generic, the idea was, that if someday you wanted to add a new type of comments (or to let the user create/edit/manage is own type of comment) it would be "easy" to do... but, of course if it's not something that you would like, we can totally remove that part and "hardcode" everything... it was just an idea
2. I asked me the same question, and decided that storing string as key was maybe better for the case where someday you maybe wanted to change from an enum to a class/struct or whatever (we only keep to reference a "primary key/pointer" to the "object") and it was a little bit easier to store thing with string. It could be also easier if someday you wanted to make you plug-in totally configurable by the user (adding keyword by configuration)

Finally, the 1 and 2 are somewhat linked together... but of course if you know a better way with less code... i would happily learn :-)

Todos

1. The way a save/load into/from the storage is bad, it need to be better (i think)
2. Strange bug in options, when going from one tab into another, making changes, then saving, all the changes are not saved

Edit : Questions parts

[omsharp]

• For the XAML utils, just put them in Options/Infrastructure for now.

• I’m still a beginner in XAML development and you seem much more comfortable with it, so if you think that’s the best way to go then we’ll do it that way.

• For the saving mechanism, I would have to agree with you on that, there must be a better way to do it, especially when it gets bigger and more complicated. But that’s the best way I could come up with at the time, maybe will change it in the future.

• I think the settings object should be shared between the two pages. I think this might solve the bug, not sure though!

[Angelinsky7]

I think the settings object should be shared between the two pages. I think this might solve the bug, not sure though!

i think that the Settings class should maybe become a singleton. Like that, there will be only one instance of him across all the extension.

[omsharp]

i think that the Settings class should maybe become a singleton. Like that, there will be only one instance of him across all the extension.

Yeah, I think this might be a good idea!

[Angelinsky7]

i let you try and tell me what's missing (naming convention for example)

[omsharp]

Did the singleton solve the settings bug?

[Angelinsky7]

Did the singleton solve the settings bug?

Yes it did.

[Angelinsky7]

Another question : Is the special parameter for the todo (HighlightTaskKeywordOnly) should be change for all keyworkd ?

[omsharp]

HighlightTaskKeywordOnly is only for Task comments. It doesn't affect the other types.

[omsharp]

Can we make sure the user doesn't use === and &&& as tokens?

If they did I think this might break the logic in SettingsStore, because you're using === and &&& as string separators.

I think even if they use = or & only this might be an issue.

Maybe we should count for that somehow.

[omsharp]

Now that I think of it. The conversion logic between Dictionary<string, string> of tokens into a string and vice versa should be in the Settings class and not in the SettingsStore class.

The SettingsStore should only be responsible of storing and loading settings, and it should only handle the four basic types, int, double, bool, and string. Conversion is not its responsibility.

Look at my other extension Line Press, I've done a similar thing there.

[Angelinsky7]

If you look at SettingsStore now you can see that i don't use a string conversion anymore (so no need to worry for the "===" and "&&&") I fortgot to remove the old const, now it's done. Yes, we can move the all logic one step higher but if you look at the code (b29116f), it's seems to me normal to keep it there.

• before : i converted a dictionnary to a string and then save the string into the store
• now : i direclty save a dictionnary into the store (with subcollection)

HighlightTaskKeywordOnly is only for Task comments. It doesn't affect the other types.

Yes i understand, but my question was more like : What if a user would like to have the same behaviour with the Important comments now that he can customize it ? But i understand that this question leads to another : why the plugins "impose" types of comments while it could instead propose by default the actual type (with their actual behaviour) and let the user configure them how he likes them.

But i was just a thought :-)

[omsharp]

The main idea of the extension was changing the whole comment's color, to make it stand out, and HighlightTaskKeywordOnly came as a user request, and no requests were made for having the same feature for other comments. So I really don't see any good reason to go through all the change.

We can't just keep asking "What if?", that way we'll never stop.

[omsharp]

Removing the string splitting from SettingsStore is a good thing. But I'm not really happy with the way SettingsStore turned into, the switch statement in Save and Load starting to look pretty ugly. The way I intended it to be is that it only handles bool, int, double, and string, and Settings class will be responsible for handing those values. That way the switch case is kept compact and never change.

I want SettingsStore to not be changed every time we add a new type of settings.

[Angelinsky7]

I want SettingsStore to not be changed every time we add a new type of settings.

Ok, but how to you want to handle it... because the current implementation need to call and use the WritableSettingsStore

[omsharp]

I'm not sure it could be done with the current design. I'm thinking of merging this pull request as it is, for now, and then see if I can do something about it or redesign the whole thing.

[omsharp]

Could you please handle the naming convention issue, and the formating as well.

[Angelinsky7]

No problem, but could you give me an example of "convention issue" to see what is important for you ?

[omsharp]

Naming

• Private fields should be camelCase with no prefix or suffix. Other than that I don't see any naming issues. Exmaple: use targetObject instead of _mTargetObject

Format

1. Use var keyword instead of the full type name.
2. A new line for each curly braket. This rule might be ignored in simple cases (single lines).
3. Keep an empty line before and after code blocks (Methods, Properties, Ifs, Fors, etc...)
4. Keep lines as short as possible, and break into multiple lines if needed.

Here is a simple example on formating.

[omsharp]

If number 2 in format is too much work for you. You can ignore it, I'll be able to use VS's auto format to handle it. But the other points you'll have to address.

[omsharp]

Thanks for being patient and bearing with me :)

[omsharp]

You know what, you can ignore format number 2 also. I'll be able to go through your code and change it myself. So you still have 3 and 4.

[Angelinsky7]

i think we are good :-)

[omsharp]

Great. I'll work on the merge as soon as I can.

[Angelinsky7]

Thanks you :-)

[omsharp]

Welcome, and thank you for your work and patience.