search

pazz / alot

Terminal-based Mail User Agent
GNU General Public License v3.0
683 stars 163 forks source link

Make sure that we have a valid path for DBManager #1610

Closed guludo closed 1 year ago

guludo commented 1 year ago

Otherwise, we run into problems when we need to use the path attribute and it is None (for example, in DBManager.add_message()).

Getting the path directly from the notmuch config file is not enough, because the user might be using the default location with no explicit entry in the file. As such, it is better to ask it directly to notmuch.

pazz commented 1 year ago

Would it not be simpler, for this use case, to hard-code the same default value as notmuch does as a fallback argument to settings.get_notmuch_setting? I mean, if that value is not in notmuch's config, the binary will also fall back to a hardc-oded default wouldn't it?

guludo commented 1 year ago

Would it not be simpler, for this use case, to hard-code the same default value as notmuch does as a fallback argument to settings.get_notmuch_setting? I mean, if that value is not in notmuch's config, the binary will also fall back to a hardc-oded default wouldn't it?

I'm not sure about that, since there isn't exactly simple hardcoded value to use. There is a logic notmuch follows to get the database path. I extracted the following from the notmuch-config(1) manpage:

DATABASE LOCATION
       Notmuch database search order:

       1. Directory specified by NOTMUCH_DATABASE environment variable.

       2. Directory specified by config key database.path.

       3. $XDG_DATA_HOME/notmuch/<profile> where <profile> is defined  by  NOTMUCH_PROFILE  environment  variable  if  set,
          $XDG_DATA_HOME/notmuch/default otherwise.

       4. Directory specified by MAILDIR environment variable.

       5. $HOME/mail

So I think it is rather simpler to just ask it directly via notmuch config instead of reimplementing that logic. And if some version of notmuch changes how it finds the database location, there is no extra work from our part.

guludo commented 1 year ago

Oops. I just realized there is an issue with the current patch, forgot to remove the line indexpath = settings.get_notmuch_setting('database', 'path'). Will send a new version with that removed.

guludo commented 1 year ago

Oops. I just realized there is an issue with the current patch, forgot to remove the line indexpath = settings.get_notmuch_setting('database', 'path'). Will send a new version with that removed.

Force-pushed with the change above. I also took the opportunity to elaborate more on the reason for using notmuch config in the commit message.

pazz commented 1 year ago

It's taken me a while but I am generally on board now with calling the notmuch binary for stuff. However, I'd much rather have this done systematically and not just hardcoded for this one use-case.

Also, the main UI code as well as the settings manager are already too bloated, and all this could use some refactoring. I am not sure how best to address this but some other use cases for calling the notmuch binary are:

guludo commented 1 year ago 
* reading notmuch config settings, instead of parsing its config fir directly)

Doing this one with #1615.

guludo commented 1 year ago
  • making tagging operations; This is sometimes error prone as done now. For example, I keep seeing individual mails in my inbox and alot's untagging mech does not seem to work for those messages. Occasionally I then just call notmuch on the command line. We could simply do this all the time, or at least have this as an option in alot.

Do you know of a way we can reproduce it? Is there an issue already created for this? I'm thinking of closing this PR, since #1615 was merged.

pazz commented 1 year ago

Unfortunately not. I will try to investigate next time it happens but it is rare and seemingly nondeterministic. Yes, let's close this issue for now and open another one if needed for the mentioned bug. Cheers!

close

Quoting Gustavo José de Sousa (2023-04-16 15:39:53)

  □ making tagging operations; This is sometimes error prone as done now.
    For example, I keep seeing individual mails in my inbox and alot's
    untagging mech does not seem to work for those messages. Occasionally I
    then just call notmuch on the command line. We could simply do this all
    the time, or at least have this as an option in alot.

Do you know of a way we can reproduce it? Is there an issue already created for this? I'm thinking of closing this PR, since #1615 was merged.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.*Message ID: <pazz/alot/pull/1610/ @.***>