Discussion: major refactor

[benrhughes]

---

@mjdescy has kindly volunteered to do some major work on todotxt.net, including some fundamental changes. This issue will be used to publicly discuss how things will proceed. Below are @mjdescy's ideas - we'll use the comments for further discussion (anyone interested is welcome to join in).

---

Here is what I envision doing:

1. Open an Issue on GitHub for each feature I create or bug I fix. This will make everything publicly documented.
2. Work on changes in the "dev" branch to add features and close bugs prior to enabling multiple selection.
3. We should define a feature cutoff and push out a new release prior to implementing multiple selection. This release would be the last of the planned version 2 releases.
4. You (@benrhughes) would create a release.
5. Create a feature branch for multiple selection. This may be a long-running branch (1-3 months) and I'd like to keep it separate from the dev branch until we are comfortable with it. Multiple selection will require some architectural changes to the application.
6. When multiple selection is working the way we like, we can merge the feature branch to the dev branch.
7. You (@benrhughes) could create a release of the multiple selection version, which may be version 3.0.0.

---

I think it makes sense to make the list view keep track of tasks by "task ID" (the position in the todo.txt file) rather than by "rawText". That could help make the selection behavior work more consistently when there are tasks with the same text in the task list.

Would you be amenable to using Todotxtlib.net (https://github.com/hartez/todotxtlib.net) as the base library? I haven't done a deep code review of it yet, but it may provide a better base for an app that does batch updates. Whether we use that library or not, I think we should plan on doing a batch update of the Tasklist in memory, then flushing it to disk, rather than updating and saving the file for each task updated.

---

As for the UI, I don't plan on major changes, but there are some minor changes I would like to make.

1. Add a Task menu, mostly for accessibility and to provide a place for the user to look up the keyboard shortcuts.
2. Add an Edit menu, again for accessibility. The Edit menu would just be for copy/paste, select all, etc.
3. Add some new preferences to make some of the more esoteric features optional. Examples include: appending applied filters to new tasks, and indenting tasks when the task list is grouped.

[benrhughes]

Process

I'm completely comfortable with your workflow suggestions - it's how I've been doing things as well. I agree multiple selection is a good time to cut v3.0

Using an ID

1. While it solves the (rare) case of multiple tasks with exactly the same text, it doesn't solve the grouping selection issue - if the same task appears in multiple groups, each instance will still have the same ID, and therefore be selected
2. If you do find a need for it, you need to be careful about how you do it. Keep in mind that the todo.txt file can change underneath you (via a dropbox sync). If you assign ids based on file position, a sync can mess things up.
3. As an aside, this is also why the current implementation is so I/O heavy: its the simplest way to ensure that the version in memory is accurate. People can (and do - because I've had bugs relating to it) update their todo.txt from multiple machines and they expect the syncing to be seamless. Any task change needs to be written to disk as quickly as possible. It's quite likely you can come up with a smarter way of doing it than I did, but just be careful to not cause any unintended sync issues

todotxtlib

I don't in principle have anything against using it. At one stage, early on, I looked at doing the same thing. I do like the idea of having a canonical .NET todo.txt parsing library.

However, when I looked at it, I had some reservations. As above, I have concerns about the buffer-and-flush approach to the file interaction. IIRC todotxtlib was designed more as the backend to a WP7 app, so the dropbox sync may be under the app's control. In our case, the sync occurs completely outside of our control - all we can do is react to it when we detect it.

I also didn't particularly like the way that the parsing was implemented, though I can't remember why. It very may have improved since I looked at it.

If you think you can get it to work as we need it to though, I'm happy enough for you to bring it in.

UI

I'm happy with the changes you've suggested. My guiding principle (which I have sometimes broken!) is to only implement the official task syntax, but I'm fairly open about non-syntax helpers like those you've suggested, so long as they don't clutter the main UI.

[mjdescy]

Those are a lot of good points. The most conservative way forward, in my opinion, is to try to close some of the UI bugs and add some useful features to the current single-selection version of todo.txt, without changing the base Task and TaskList classes too much. The app works just fine as it is in terms of updating the todo.txt file one task at a time.

Part of that process of moving toward multiple selection would involve refactoring the code base. Primariliy, I would like to move away from monolithic event handler methods that implement every feature, and toward a state where we have numerous small methods, one for each function. When that is done, it will be easier to modify the code base to support multiple selection and batch task updates.

[benrhughes]

I would love to see the even handling refactored. It's by far my most hated part of the code. It's one of those things that started simply enough that grew into a giant mess :)

In terms of the file interaction, if you can get it to work well within our constraints, I don't mind if you change it to buffer-and-flush. As I said, it's just something to be mindful of.

[mjdescy]

I am glad to hear that. I was working on such changes in my local repo, mostly to learn how to do RoutedUICommands in XAML, and thought I might have to implement the changes after doing other things. Instead, I think I will implement them sooner rather than later, so the new features I write won't have to be refactored. It's going to be one heck of a commit!

Buffer and flush should work fine for file interaction because the commands will be executed in milliseconds. One hurdle to get over, though, is refreshing the file prior to updating it. I also need to explore how the grouping functionality--especially how one task can appear in multiple groups--would be affected by batch updates and multiple selection. It's a great feature, but it is a little mysterious to me still because I have not looked at its code.

[mjdescy]

I am going to continue to identify small changes and add issues to document what I plan to change.

I am looking into a better way to refactor the commands than RoutedUICommand, perhaps using RelayCommand or DelegateCommand. I need to do more research and choose the approach that would reduce the code base the most, while remaining easy enough to understand. As an intermediate step, I can create public methods in the MainWindowViewModel for all the commands. In doing so, I can encapsulate the code without changing it much, prior to remapping how the commands are executed.

[mjdescy]

I am doing the refactor in a stepwise basis. I thought it would be easier for everyone to understand the changes that way. Also, it makes it easier to isolate any regressions that might be introduced (hopefully none).

I am leaning toward implementing the menu and keyboard shortcut commands using RoutedUICommands defined in MainWindow.xaml. That approach is very similar to the event-based way things work now, and it is a bit cleaner because we can get rid of most of the keyboard event-handling methods. The main drawback is that it requires a bunch of one-line methods in the window code-behind to call the methods in the MainWindowViewModel. It is very easy to understand what's going on in that code, however, so we will still come out ahead.

[mjdescy]

@benrhughes Do you have any objections to updating the .NET version supported from 2.0 to a higher version? I don't have specific plans yet, but I am looking into ways to retain the selection after updating or deleting multiple tasks, and some of the options are off the table if we're basing the app on the .NET 2.0 framework.

[benrhughes]

Pretty sure it already targets .NET 4?

On Thu, May 15, 2014 at 4:45 AM, Michael Descy notifications@github.comwrote:

@benrhughes https://github.com/benrhughes Do you have any objections to updating the .NET version supported from 2.0 to a higher version? I don't have specific plans yet, but I am looking into ways to retain the selection after updating or deleting multiple tasks, and some of the options are off the table if we're basing the app on the .NET 2.0 framework.

— Reply to this email directly or view it on GitHubhttps://github.com/benrhughes/todotxt.net/issues/200#issuecomment-43120954 .

[mjdescy]

My mistake. It is .NET v4.0.

[mjdescy]

I have stalled out on this for non-coding related reasons. I do think that getting the selection logic right is a major PITA, though. :-) I hope to get working on multiple selection again soon.

[mjdescy]

I just pushed out a new branch for multiple selection testing, and for other improvements on the way to version 3.0.0. We can merge that branch into dev once we are comfortable with it.

[benrhughes]

Sorry Michael, it's been pretty full on here so this completely slipped through the cracks.

I've got a bug fix (#228) that I'd like to get out soon. I'll make some time to test your multiple changes. If they look good I'll get my fix in and cut a 3.0, otherwise I might do a 2.5.1 while we work through any issues.

Cheers,

Ben

On Thu, Jun 26, 2014 at 1:22 PM, Michael Descy notifications@github.com wrote:

I just pushed out a new branch for multiple selection testing, and for other improvements on the way to version 3.0.0. We can merge that branch into dev once we are comfortable with it.

— Reply to this email directly or view it on GitHub https://github.com/benrhughes/todotxt.net/issues/200#issuecomment-47184064 .

[mjdescy]

Ben,

Sorry I didn't respond to this comment sooner. I was on vacation.

I have been testing all the changes I committed to development (multiple selection and multi-level sorting) for many weeks now and haven't come across any bugs or crashes. I think both changes really make the application do exactly what I want now, and that many users will be happy with them.

I did not make fundamental changes to file I/O or to any other fundamental assumptions about the state of the todo.txt file. That should make everyone happy. The biggest behind-the-scenes change to enable multiple selection was updating the logic that preserves the user's selection(s) after changes are made.

I would love the focus bug (#228) fixed! That is actually really exciting to me.

Please take as much time as you need in preparing a release. If you have any questions for me about the code I committed, don't hesitate to ask.

[mjdescy]

@benrhughes Any thoughts on when you'd be ready to make a new release with the changes I coded?

[benrhughes]

Hi Mike, I'm really sorry about the delay. Since my 4th was born, I haven't really made any time for any of my side projects.

I'll aim to get the release done today.

Cheers, and again, sorry for the huge delay.

Ben

On Mon, Oct 27, 2014 at 8:12 AM, Michael Descy notifications@github.com wrote:

@benrhughes https://github.com/benrhughes Any thoughts on when you'd be ready to make a new release with the changes I coded?

— Reply to this email directly or view it on GitHub https://github.com/benrhughes/todotxt.net/issues/200#issuecomment-60532524 .

[mjdescy]

No worries, Ben. I totally understand. I only have 1 kid, and wasn't able to touch TodoTxtMac in about 6 months! :-)

[Piscean]

I just grabbed the 3.0 source to add some of my personal changes and notice the ToDoTests project no longer builds. 106 errors, mostly missing type or namespace errors (NUnit, TestFixtureAttribute, Assert, etc.) I never build the Test directory, but some might.

[benrhughes]

The tests built for me a couple of days ago (I build and run them before release) but perhaps the nuget stuff isn't set up properly to pull down dependencies. I'll take a look when I get a chance. On 01/11/2014 12:13 pm, "Piscean" notifications@github.com wrote:

I just grabbed the 3.0 source to add some of my personal changes and notice the ToDoTests project no longer builds. 106 errors, mostly missing type or namespace errors (NUnit, TestFixtureAttribute, Assert, etc.) I never build the Test directory, but some might.

— Reply to this email directly or view it on GitHub https://github.com/benrhughes/todotxt.net/issues/200#issuecomment-61351438 .