Option to Sort Notes on Export + Config UI

[ohare93]

This PR is an attempt to fix Issue #65

I may have went a bit overboard from what we talked about, but with this change it is possible to sort by up to 4 keys! :grin: To add another sortable value one need only add a viable option to the sorting_definitions :+1:

https://github.com/ll-in-anki/CrowdAnki/blob/9834c199ee0e5eb2957d2bf2803bf4ed9e33370b/crowd_anki/export/anki_exporter.py#L101-L109

Let me know what you think!

Still to do

• Documentation
• Unit Tests

[ohare93]

This PR is an attempt to fix Issue #65

[Stvad]

A config validator would be acceptable 😁

It can even do some guessing if you'd like (I'm not convinced that it's really required besides basic .strip.tolower), as long as it's not inlined with the logic of sorting.

A somewhat relevant link on the topic: https://en.wikipedia.org/wiki/Levenshtein_distance

[ohare93]

Pushed a change, all the functionality is now in a NoteSorter class. I think you'll agree it's a lot nicer now, all the settings are calculated in the constructor, and the sort_notes function is down to 16 lines long!

Still to do: 1) Move config settings out of NoteSorter :thinking: 1) Unit Tests for NoteSorter(much easier now!) 1) Documentation

May do for this PR:

• Make a config validator, which occurs on change of the config settings

I have some other changes I wish to implement to CrowdAnki, such as fixes for Issue #31 and Issue #23. Those would need to use the config settings to some degree, and thus would benefit from an improved custom UI for the config, which would also easily allow for validation. I too would like to remove the validation from the NoteSorter, but I do not know if I will get around to it for this PR if the config UI takes too long. If not, I will remove it as part of one of these other changes in the coming month :+1:

[ohare93]

Got some help from a friend who's also working on Anki extensions. He steered me in the right direction for QT UI files, and so it was simple to make one for the config page. Behold (a work in progress)!

I'm still connecting up the text boxes to their respective config values, so the page is still a work in progress. But the checkboxes are fully functional, and change their config values.

When the user hits "Okay" then validation can be done, and will not close the window or save the info the config if the values are incorrect. I have made dedicated classes and enums for the Config data info, and Sort Method options, which will make that simple.

Let me know what you think :grin:

[ohare93]

Update: all config values are now connected up through the GUI to the config file itself :+1:

The Note Sort Type box also has input verification upon hitting "Ok", not allowing the user to save until only correct values remain.

It will be a lot easier in the future to add config values :grin: Simply add a checkbox/textbox in QT Designer, build the UI file to Python, connect the ui items to the config file itself in config_settings.py, then just use it!

[ohare93]

Spread the ConfigSettings class to the rest of the code, now the ActionVendor and HookVendor simply use the values ConfigSettings class which is passed to them, which handles all the default values and reading the config file and whatnot.

Also finished some Unit Test for each filter options, to make sure it returns the cards in the right order :+1:

I believe with this I am 99% done! :grin: I'm sure there's some more bits and bobs lying around, but please do give some feedback (whenever you're not too busy with travelling) :+1:

[Stvad]

wow, that's a lot of nice things :) I'm heading back home now, so should be able to give you some feedback soon.

[ohare93]

wow, that's a lot of nice things :) I'm heading back home now, so should be able to give you some feedback soon.

Ayy you're back! Hope the holiday went well :+1:

[ohare93]

Made the changes you suggested :+1: I expect you aren't properly home yet and haven't played around with the new stuff, so give it a shot whenever you're free and get back to me. When I am assured this PR will/does go through and the new Config settings will be used in their current form then this new easier way of creating config settings will allow me to quickly and easily make a few others changes.

I plan to: 1) Make a simple fix for #23 with just a checkbox in the config 2) Try and tackle my own issue #62 -- no idea how long this one may take, or what the config options will look like until I try it 3) See what I can do about issue #31. This one I imagine you will have lots of opinions about, as it's simple enough to give the user the option to not export deck headers, but then I'd need to make it possible to import without them, and also perhaps add in a config setting for "Ignore deck header on import". Lots of possible things here.

Issue #22 looks simple enough to fix too. Anyways, thanks for all the feedback, and for making this useful program in the first place! I hope I can do whatever I can to make it a bit better :grin:

[Stvad]

Just loaded the UI, some notes:

• neat!
• how about using a multi-select for sort options (e.g. with List widgit or something). (The root decks should probably be the same, but that's likely more involved and out of scope here)
• how about using QFileDialog for selecting the snapshot path?
• section header labels should probably be larger, they are dominated by other elements on my screen

[Stvad]

btw, as I loaded it locally - Pycharm tells me that there is a variety of pep8 violations - please look into fixing those

[Stvad]

Made the changes you suggested 👍 I expect you aren't properly home yet and haven't played around with the new stuff, so give it a shot whenever you're free and get back to me. When I am assured this PR will/does go through and the new Config settings will be used in their current form then this new easier way of creating config settings will allow me to quickly and easily make a few others changes.

I plan to:

1. Make a simple fix for #23 with just a checkbox in the config

2. Try and tackle my own issue #62 -- no idea how long this one may take, or what the config options will look like until I try it

3. See what I can do about issue #31. This one I imagine you will have lots of opinions about, as it's simple enough to give the user the option to not export deck headers, but then I'd need to make it possible to import without them, and also perhaps add in a config setting for "Ignore deck header on import". Lots of possible things here.

Issue #22 looks simple enough to fix too. Anyways, thanks for all the feedback, and for making this useful program in the first place! I hope I can do whatever I can to make it a bit better 😁

awesome! I really appreciate your contributions to making CrowdAnki better! One thing for the future - I'd advise splitting up the work in a smaller chunks so we both can iterate faster on it 🙂

[ohare93]

I somehow missed this comment! 😅

• how about using a multi-select for sort options (e.g. with List widgit or something). (The root decks should probably be the same, but that's likely more involved and out of scope here)

Yea that's a lot harder than it sounds, since you can have a variable number of sort options. Either multiple sort options multi-select boxes are shown (an explicit set amount) or new ones are generated as methods are selected 😓 (and checks are made to not be able to select the same thing in each)

• how about using QFileDialog for selecting the snapshot path?

Good idea 👍

• section header labels should probably be larger, they are dominated by other elements on my screen

Jesus that is not how it looks on my screen! 😅 Here's how it looks to me:

Making the headers bigger should be fine though.

[Stvad]

I entirely agree about splitting this all up! I hope you can see what led me down this rabbit hole with the config settings in the first place though, and how it was intertwined with how the NoteSorting would happen 😅 now that the config settings are in place I can make future changes much much smaller in scope 🤞

Yep, I saw that 🙂

I also made this a review, so I can drop all my comments on you in one smooth reply (and only one email! 😅) This is the time I wake up at (2 hours ago that is) and it seems you are quite active now, so feel free to hit me up if you'd like a simple chat about stuff in the future 👍

Yeah.. review implies that I'd be able to finish it in 1 sitting which is a bit hard with the current size of the PR 😉 so expect a lot of singular commits from me yet 😜. Will reach out to you over email to find a commonly used IM, for better instant communication in the future :)

[Stvad]

Jesus that is not how it looks on my screen! 😅 Here's how it looks to me:

Hmm, some of it is platform differences probably, but the proportions differences are weird 🤔

[Stvad]

I have been quiet lately as I've been busy, but can get back to this soon.

👍

I also switched over to PyCharm, which has helped in development but been a time sink.

PyCharm is nice :). Why was it a time sink?

I've checked in fixed for the Python violations. Any remaining were in your original code, or are necessary ones to be ignored (accessing a private module). Let me know if you disagree with that assessment 😉

Feel free to fix any violations in my code that you see. I believe i haven't added any of them consciously besides the following 2 (should probably add this to Contributing doc):

• Anki interaction points require violations at times, as anki codebase does not follow pep8
• My preferred line lenght is ~120 and not 80.

Been over all your comments, have a few more tasks to get done and then let's see how pullable this monster is. I admit, I am starting to get fed up with it 😅 😏

We're getting there ;) I appreciate you working on the feedback and contributing in general!

[Stvad]

Btw if there are any things that were non-obvious/you think can get other people with contributions - please add them to Contributing.md

[ohare93]

PyCharm is nice :). Why was it a time sink?

Just to get used to it, and get my setup working again like I had on VS Code, with auto completion and linting for Anki's code. I still don't have it so I can run/debug directly from PyCharm (I can't work out the damn run config settings needed!) like I had in VS Code, but I can get around that for now.

Feel free to fix any violations in my code that you see. I believe i haven't added any of them consciously besides the following 2 (should probably add this to Contributing doc):

• Anki interaction points require violations at times, as anki codebase does not follow pep8
• My preferred line lenght is ~120 and not 80.

:+1:

We're getting there ;) I appreciate you working on the feedback and contributing in general!

:100: :+1: :grin:

[ohare93]

Bunch more changes, replied to a lot of comments too. Getting through it! :sweat_smile:

[ohare93]

Had a spare half hour, managed to get an example of multi note sorting methods testing. I'm sure there's some Python magic I can do to test every one of the combinations, but my mind is blown for today :hear_no_evil:

Progress tho!

[Stvad]

Awesome! :) I don't think all combinations are required ;)

[ohare93]

Checking some tests for Config_Settings :+1:

One test is commented out, as I can not get it to work :thinking: It is to test the saving, however the _config file gets set to a MagicMock from Anki's getConfig function, and I cannot make these damn mocks return the actual value of what I give them :sweat_smile:

[Stvad]

_config is not a file it's a dictionary. So you can set it to an empty dict or something before calling save.

Another option which I'd probably find more preferable is do what we discussed before when there was a guard flag on the __init__ of ConfigSettings - to pass in the dictionary and load things from anki only if it's not provided. In that way you'd be able to do other testing in more realistic fashion as well as your code flow would be same for tests and irl.

So the init method would look like:

    def __init__(self, config_values = None):
        self._config = config_values or mw.addonManager.getConfig(__name__)
        self.load_values()

[Stvad]

Also that would allow to properly engage/test the whole default loading mechanism

[ohare93]

_config is not a file it's a dictionary. So you can set it to an empty dict or something before calling save.

Another option which I'd probably find more preferable is do what we discussed before when there was a guard flag on the __init__ of ConfigSettings - to pass in the dictionary and load things from anki only if it's not provided. In that way you'd be able to do other testing in more realistic fashion as well as your code flow would be same for tests and irl.

So the init method would look like:

    def __init__(self, config_values = None):
        self._config = config_values or mw.addonManager.getConfig(__name__)
        self.load_values()

Yea good call. Done :+1: Test works happily now

[ohare93]

Singleton-ness fixed :+1: I do not believe it actually caused any issues before, but it's always good to stay ahead of ourselves.

Turns out the issue with the automated snapshot was there already, as hooks are only setup when the options was True on startup. I have changed the HookVendor to always setup the hook, but for the ArchiveVendor function to check the config setting as it tries to run (it already had access to config, so it was a minor change). Have tested it manually myself, and snapshots work on switching profile now, regardless of what your setting was initially :grin:

This also made the hook_vendor_spec useless, as there is nothing to test, so I deleted it. Considered adding a similar test for ArchiveVendor now, but it looked hard, and I'd rather get your opinion on the change before going too far :grin:

[Stvad]

Turns out the issue with the automated snapshot was there already, as hooks are only setup when the options was True on startup.

That's fair 😛. I picked that one randomly though, as an example of general principle (not a very lucky one apparently 😃). If my model of the world is correct it'd be still broken without the change to use singleton instance everywhere

[Stvad]

Btw as we're getting close - you should rebase on mainline, as github tells me that there are conflicts ;)

[Stvad]

Giving it another full review now

[ohare93]

Giving it another full review now

Sounds great :+1: I shall have time to review your review tomorrow or so, so take your time.

Btw as we're getting close - you should rebase on mainline, as github tells me that there are conflicts ;)

Merged in the changes from master :+1: Was just the Pipfile.lock that was the issue, so no big deal.

[Stvad]

Sooo close! 😁

Indeed, I feel like we can conclude things today :)

[ohare93]

Sooo close! grin

Indeed, I feel like we can conclude things today :)

:100: :ok_hand:

Minor changes complete. I am available for the next 3 hours, then I am gone for the day :slightly_smiling_face: If there are any small change left let me know and I shall get it done! :+1:

[Stvad]

Going to sleep now myself. Gonna take a final look tomorrow, and hopefully merge it :)

[Stvad]

Ah, I believe you haven't actually addressed the UI improvements we've discussed, oh well, I'll create an issue to track that.

UPD: Created #68

[Stvad]

Merged! Thank you for contributing @ohare93 !