Create interactive command for curses interface (for discussion purposes) #66

Pi2048 · 2017-01-11T00:15:41Z

This is my first attempt at a curses-based interface for Todoman. I would like your feedback on what I've written so far. I am aware that it is by no means done, but before I start polishing it I would like to know I'm polishing the right thing. This PR is in reference to issue #62.

To run:

Create a virtualenv as you would for development purposes
Run todo interactive

In the database listing view (the one you see when starting the program), the following commands are available:

l: ListChooser, choose a different database from the available ones
e: Edit, edit the currently highlighted item
n: New, create a new item in this database
m: Move, move the currently highlighted item to a database
c: Copy, copy the currently highlighted item to a database
d: Delete, delete the currently highlighted item
D: Delete all completed, delete all tasks in the database that are completed
h: Hide or show completed tasks in the database
j and k are like 'up' and 'down', like in vim
space or enter toggles the currently highlighted item

In all screens, esc closes the current screen and returns you to the previous one.

I have one specific issue on which I would appreciate your views. When I create a new item, the current database should be updated. However, you defined the todos property of Database as a cached_property, which prevents this. I have, for now, changed the cached_property into a property, but I have already noticed this is a severe performance impairment. Could we resolve this situation in a neater way? E.g., could we explicitly add the new item to the cached database?

The following definitely still needs to be done:

Help screen
Status bar messages
Some configuration options (hide_completed, default_list, sort_order)

I would also like to make a page that shows all upcoming tasks across databases.

Pi2048 · 2017-01-11T00:37:54Z

First bug found: if I create a new item, it defaults to completed, without regard to the information the user provided in the editor.

WhyNotHugo

Running todoman interactive just shows me the statusbar on top that says Todoman, and nothing else on screen.

I'll try too look at it closer and find out what's going on when I get a bit more time.

The code looks tidy enough, though I've left some minor comments.

WhyNotHugo · 2017-01-11T01:07:13Z

todoman/interactive.py

+
+logger = logging.getLogger()
+
+class TodomanItem(urwid.CheckBox):


TodoItem might be more descriptive.

As for other classes, prefixing Todoman to their name sounds redundant (especially since they're already in package todoman.interactive.

WhyNotHugo · 2017-01-11T01:08:33Z

todoman/interactive.py

+        TodomanItem (i.e. self) into a string representation that is suitable
+        for its context.
+
+        (TodomanItem, str, Database, function) -> None


We're using sphinx-like doc for param/return types:

http://www.sphinx-doc.org/en/stable/domains.html#info-field-lists

It's nitpicky of me, but let's keep it consistent.

WhyNotHugo · 2017-01-11T01:24:40Z

todoman/interactive.py

+        if key == 'l':
+            self.list_chooser()
+            return None
+        if key == 'm':


Use elif in all these and avoid all the extra return None lines.

WhyNotHugo · 2017-01-11T01:29:57Z

todoman/interactive.py

+        (TodomanInteractive, str) -> None
+        '''
+        if key in ('q', 'Q'):
+            raise urwid.ExitMainLoop()


When editing an entry, if a checkbox if focus and I press q, it immediately exists, without any confirmation, etc.

WhyNotHugo · 2017-01-11T01:32:01Z

todoman/model.py

@@ -237,7 +237,7 @@ class Database:
    def __init__(self, path):
        self.path = path

-    @cached_property
+    @property


You'll need to manually cache this to avoid the extra overhead.

That is:

Set _cached_todos on first read.

Manually add new entries there.

Manually remove entries from there.

Return _cached_todos if it's defined, otherwise cached all of them.

I think this was done to keep the database fresh even after editing an item. Let's make todos not a property, but instead rename it to get_todos with a skip_cache parameter.

WhyNotHugo · 2017-01-11T01:35:12Z

Also, please rebase onto master and add yourself to AUTHORS.

untitaker · 2017-01-11T10:15:38Z

I'd prefer if the interactive interface does deletions and marking things as done upon exiting the interface, so I can hit ^C to discard those changes.

untitaker · 2017-01-11T10:17:00Z

todoman/cli.py

+    Provide an interactive, curses-based interface to Todoman.
+    '''
+    TodomanInteractive(ctx.obj['db'].values(), ctx.obj['formatter'])
+    ctx.exit(1)


Why always exit with 1?

untitaker · 2017-01-11T10:21:09Z

todoman/interactive.py

+        A default callback function to use when closing the previous page. 
+        This callback function handles several keywords that are generic to
+        all callbacks. If a certain keyword is not set, the method does
+        nothing with it.


I think pages could be implemented much more easily, without subclassing. You'd need a wrapper widget that catches Esc (so just let the keypress event bubble up from the page's handler in that case), and that widget maintains a stack (list) of pages that it appends and pops from. If the list is empty, ExitMainLoop is raised.

untitaker · 2017-01-11T10:21:47Z

todoman/interactive.py

+
+        (TodomanItemListPage, **kwargs) -> None
+        '''
+        for key, value in kwargs.items():


Same here, why not just have database as part of the method signature?

untitaker · 2017-01-11T10:24:56Z

todoman/interactive.py

+        '''
+        self.filename = filename
+        self.database = database
+        try:


Might as well have the params database, todo instead of database, filename and avoid this logic.

untitaker · 2017-01-11T10:25:31Z

todoman/interactive.py

+        items = []
+        for filename in self.database.todos.keys(): 
+            todo = TodomanItem(filename, self.database, self.generate_label)
+            if not self.done_is_hidden or not todo.is_completed:


We should reuse the logic from the CLI here, but not sure how.

untitaker · 2017-01-11T10:27:20Z

todoman/interactive.py

+        newPage = TodomanDatabasesPage(self.parent, self.callback_copy_to)
+        self.open_page(newPage)
+
+    def delete(self, item = None):


Please no spaces between = when part of parameters.

untitaker · 2017-01-11T10:28:17Z

todoman/interactive.py

+
+        (TodomanItemListPage, TodomanItem) -> str
+        '''
+        return "{0} {1}".format('!' if item.has_priority else ' ',


We already have todoman.ui.TodoFormatter for this (which will also add colors)

untitaker · 2017-01-11T10:30:03Z

todoman/cli.py

+from .interactive import TodomanInteractive
+
+import logging
+logging.basicConfig()


basicConfig should be called in the cli function, not at module level. In the case of showing a UI you won't want to output anything I think.

untitaker · 2017-01-11T10:30:19Z

todoman/cli.py

+
+import logging
+logging.basicConfig()
+logger = logging.getLogger()


Use logging.getLogger(__name__) to namespace your logging.

untitaker · 2017-01-11T10:34:47Z

todoman/interactive.py

+
+        (TodomanPage, **kwargs) -> None
+        '''
+        for key, value in kwargs.items():


Isn't it invalid to call this callback with anything but statusbar?

If yes: def callback(self, statusbar)

If no: def callback(self, statusbar, **kwargs)

untitaker · 2017-01-11T10:36:12Z

I generally feel like a lot of classes are unnecessarily prefixed by Todoman.

untitaker · 2017-02-13T11:48:33Z

@Pieter1024 it's been a while. Are you still willing to work on this?

WhyNotHugo · 2017-02-13T12:30:09Z

Sorry, I really broke this PR with some substantial changes to todoman.
I'll can rebase it into master if you're still interesting on working on it, @Pieter1024.

WhyNotHugo · 2017-02-18T16:22:50Z

I've rebased this to master and fixed style violations, but haven't really looked at the code more closely. Since @Pieter1024 hasn't responded back, will probably be picking this up in the coming days, if time allows.

untitaker · 2017-02-18T17:17:54Z

I'll make a separate PR for the logging.

untitaker · 2017-02-18T17:21:55Z

See #106

WhyNotHugo · 2017-02-18T17:50:08Z

todoman/interactive.py

@@ -530,7 +530,8 @@ def __init__(self, databases, formatter):
        Create a Main instance based on the Database objects that
        the regular Todoman cli module passes.

-        (Main, [Database], TodoFormatter) -> None
+        :type databases: list[Database]


We still need to change this, because with the current model it doesn't make sense to pass a list of Databases.

WhyNotHugo · 2017-02-18T18:02:44Z

todoman/interactive.py

        '''
-        return self.todo.priority not in [None, 0]
+        return self.todo.priority not in (None, 0)


Maybe it's time to implement Todo.is_urgent as a wrapper around this, so we stop duplicating todo.priority not in [None, 0] al over the place.

Also, I think it can never be None, because of how the getter is written.

We can remove this if we use the normal formatter instead of generate_label

WhyNotHugo · 2017-02-18T18:03:47Z

todoman/interactive.py

@@ -167,7 +156,9 @@ def __init__(self, parent, callback, database):
        is responsible for passing information to the underlying Page.
        It is usually a method of the underlying Page.

-        (ItemListPage, Main, function, Database) -> None
+        :param parent: Main


That's not the right format, it's :param TYPE NAME, for example :param int amount.

WhyNotHugo · 2017-02-18T18:07:40Z

Thanks. I also think we should shuffle packages around:

ui: has some interactive stuff, and formatters.
interactive: has more interactive stuff.

I think this should end up in:

ui/interactive (don't care which name): all the interactive stuff.
formatting: Just the formatters (shared between cli and interactive).

untitaker · 2017-02-18T18:19:26Z

Move TodoEditor into this module, rename ui to formatter and move cli, formatter, interactive into a new ui subpackag

untitaker · 2017-02-18T18:32:54Z

I just tried to refactor and bumped into a few design limits of the current datamodel

WhyNotHugo · 2017-02-19T14:35:37Z

Proposal seems great.

Do these limitations block the refactor? Or are they for future reference?

untitaker · 2017-02-19T14:36:55Z

I think so, the API isn't very pleasing for this usage.

Particularly the split between Todo and FileTodo is problematic.

WhyNotHugo · 2017-02-19T15:32:33Z

Maybe I'll have to revisit that. Having a separate class keeps the door open for separate storage types (will we ever want that!?). And I think is a good abstraction.

Maybe having the save logic in the database is a better choice (as far as abstraction goes), but we need to track "read from file" vs "read from cache". Maybe a flag in Todo will do (readonly?).

WhyNotHugo requested changes Jan 11, 2017

View reviewed changes

untitaker reviewed Jan 11, 2017

View reviewed changes

Create interactive command for curses interface

a2b1aac

WhyNotHugo force-pushed the interactive-mode branch from ad2b0f5 to a2b1aac Compare February 18, 2017 16:21

squashme: minor cleanups

01d7556

untitaker added 4 commits February 18, 2017 18:43

Mass renaming

c5524a3

Fix up docstring types

ed4dc01

Remove superfluous exit

94f991e

More code cleanup

123461e

WhyNotHugo requested changes Feb 18, 2017

View reviewed changes

untitaker added 3 commits February 18, 2017 18:52

Replace list with tuple

0a5d438

Replace leftover signatures

271bd43

Fixup

a04aa5a

WhyNotHugo requested changes Feb 18, 2017

View reviewed changes

WhyNotHugo force-pushed the master branch from e967b68 to 7ef17b5 Compare May 30, 2017 01:21

Base automatically changed from master to main March 9, 2021 18:59


		logger = logging.getLogger()

		class TodomanItem(urwid.CheckBox):

Create interactive command for curses interface (for discussion purposes) #66

Are you sure you want to change the base?

Create interactive command for curses interface (for discussion purposes) #66

Conversation

Pi2048 commented Jan 11, 2017 • edited

Pi2048 commented Jan 11, 2017

WhyNotHugo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WhyNotHugo commented Jan 11, 2017

untitaker commented Jan 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untitaker commented Jan 11, 2017

untitaker commented Feb 13, 2017

WhyNotHugo commented Feb 13, 2017

WhyNotHugo commented Feb 18, 2017

untitaker commented Feb 18, 2017

untitaker commented Feb 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WhyNotHugo commented Feb 18, 2017 • edited

untitaker commented Feb 18, 2017

untitaker commented Feb 18, 2017

WhyNotHugo commented Feb 19, 2017

untitaker commented Feb 19, 2017

WhyNotHugo commented Feb 19, 2017

Pi2048 commented Jan 11, 2017 •

edited

WhyNotHugo commented Feb 18, 2017 •

edited