Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Pi2048
Copy link

@Pi2048 Pi2048 commented Jan 11, 2017

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
Copy link
Author

Pi2048 commented Jan 11, 2017

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

Copy link
Member

@WhyNotHugo WhyNotHugo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


logger = logging.getLogger()

class TodomanItem(urwid.CheckBox):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

TodomanItem (i.e. self) into a string representation that is suitable
for its context.

(TodomanItem, str, Database, function) -> None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if key == 'l':
self.list_chooser()
return None
if key == 'm':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

todoman/model.py Outdated
@@ -237,7 +237,7 @@ class Database:
def __init__(self, path):
self.path = path

@cached_property
@property
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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

@untitaker
Copy link
Member

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.

todoman/cli.py Outdated
Provide an interactive, curses-based interface to Todoman.
'''
TodomanInteractive(ctx.obj['db'].values(), ctx.obj['formatter'])
ctx.exit(1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why always exit with 1?

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'''
self.filename = filename
self.database = database
try:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

newPage = TodomanDatabasesPage(self.parent, self.callback_copy_to)
self.open_page(newPage)

def delete(self, item = None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please no spaces between = when part of parameters.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

from .interactive import TodomanInteractive

import logging
logging.basicConfig()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


import logging
logging.basicConfig()
logger = logging.getLogger()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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

@untitaker
Copy link
Member

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

@WhyNotHugo
Copy link
Member

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
Copy link
Member

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
Copy link
Member

I'll make a separate PR for the logging.

@untitaker
Copy link
Member

See #106

@@ -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]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'''
return self.todo.priority not in [None, 0]
return self.todo.priority not in (None, 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@WhyNotHugo
Copy link
Member

WhyNotHugo commented Feb 18, 2017

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
Copy link
Member

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

@untitaker
Copy link
Member

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

@WhyNotHugo
Copy link
Member

Proposal seems great.

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

@untitaker
Copy link
Member

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

Particularly the split between Todo and FileTodo is problematic.

@WhyNotHugo
Copy link
Member

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?).

Base automatically changed from master to main March 9, 2021 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants