Change: (activities-switch) switch to activity without prompt when activites are <=2

[pizzatorque]

when activites are 2 or fewer, switch without prompt to the activity or other activity.

[alphapapa]

Hello,

I understand what you're wanting to do, but I don't think this is the best way to do it. This function is intended simply to return an activity, and sometimes it's necessary to return the current one. If you want to change the way switching works, that should be done in the activities-switch command, and it should probably be done by changing the default activity offered for completion. Maybe there could also be an option to just switch to the other activity when 2 are open.

Thanks.

[pizzatorque]

@alphapapa You are completely right about the responsibility of this being of activities-switch instead of activities-completing-read. Would the new implementation be more appropriate? I understand changing the default activity offered in the prompt, but I think it would be more efficient to switch automatically, unless the activities are 2 and you are not in an active activity.

[alphapapa]

@pizzatorque Thanks, I think this is generally okay. Suggestion: use pcase to make the code a bit more concise. :)

[alphapapa]

Here's some simple code to consider, FWIW:

@@ -400,7 +400,9 @@ (defun activities-switch (activity)
 Interactively, offers active activities."
   (interactive
    (list (activities-completing-read
-          :activities (cl-remove-if-not #'activities-activity-active-p activities-activities :key #'cdr)
+          :activities (cl-remove (activities-current)
+                                 (cl-remove-if-not #'activities-activity-active-p activities-activities :key #'cdr)
+                                 :key #'cdr)
           :prompt "Switch to activity")))
   (activities--switch activity)
   (run-hook-with-args 'activities-after-switch-functions activity))

[pizzatorque]

Sorry I have had quite the couple of weeks. I went down a pcase rabbit-hole. What do you think of this implementation? It uses both pcase and cl-remove to simplify the logic and make things a bit more clear and explicit.

[alphapapa]

No problem, there's no urgency here.

Relating to pcase, I'd suggest starting with (pcase (length activities-activities) ...) rather than using destructuring to handle different lengths.

Other than that, it would be nice to avoid having multiple forms that call activities-completing-read if possible, because that duplicates (relatively) a lot of code.

[pizzatorque]

Would you suggest wrapping that function call into another one like this? This could be used in a couple of other places

(defun activities--completing-read-actives (prompt)
  (activities-completing-read
                    :activities (cl-remove-if-not #'activities-activity-active-p activities-activities :key #'cdr)
                    :prompt prompt))

I'll adjust pcase to match the length instead of deconstructing

[alphapapa]

I'd prefer not to add another function if it's not necessary.

What about the suggestion I made here: https://github.com/alphapapa/activities.el/pull/48#issuecomment-2018237587 What problems would using that cause?

[pizzatorque]

I'd prefer not to add another function if it's not necessary.

What about the suggestion I made here: #48 (comment) What problems would using that cause?

I was able to have it work like this for switching between current activity and other

   (interactive
   (list (activities-completing-read
          :activities
          (cl-remove
           (when (activities-current) (activities-activity-name (activities-current)))
           (cl-remove-if-not #'activities-activity-active-p activities-activities :key #'cdr)
           :key #'car :test #'string=))))

This is because removing by cdr does not work when comparing the output of activities-current and activities-activities, I think the objects (pardon my French) have some differences like in timestamps that does not allow checking for equality like that. This works, but still gives the prompt, my point was more to get rid of the prompt because if I have only 2 activities I do not even need to indicate which one to switch to and can save a keypress. Shall I go with the approach in this comment?

[alphapapa]

Note that activities-activities is an alist where the keys are the names and the values are the activities.

Anyway, how about this? Seems to work well:

(defun activities-switch (activity)
  "Switch to ACTIVITY.
Interactively, offers active activities."
  (interactive
   (let* ((active-activities (cl-remove-if-not #'activities-activity-active-p activities-activities :key #'cdr))
          (selected (pcase (length active-activities)
                      (0 (user-error (substitute-command-keys "No active activities (use `\\[activities-resume]' to resume one)")))
                      (1 (user-error "Only one active activity"))
                      (_ (let ((candidates (cl-remove (activities-current) active-activities :key #'cdr :test #'eq)))
                           (pcase (length candidates)
                             (1 (cdar candidates))
                             ;; TODO: Sort by recently used.
                             (_ (activities-completing-read :activities candidates
                                                            :default (car candidates)
                                                            :prompt "Switch to activity"))))))))
     (list selected)))
  (activities--switch activity)
  (run-hook-with-args 'activities-after-switch-functions activity))

[pizzatorque]

Oooh this is nice, it's like the pcase I made but it also has error handling and only one call to the activities-completing-read, I think the :test #'eq was what I was referring about in the previous comment where cl-remove was not working by just providing the cdr key. I have updated the PR with this, thanks I feel like I have learned quite a lot just by trying to implement things as you suggested.

[alphapapa]

thanks I feel like I have learned quite a lot just by trying to implement things as you suggested.

I'm glad, as that was my intention. :)

I have updated the PR with this,

This presents a couple more learning opportunities:

1. You have git configured with an email address having @pop-os.localdomain. I don't know if that would or should be disqualifying, but the purpose of these is to uniquely identify authors, and I suppose to provide a way to contact them. Sometimes if I don't have a user's email address I'll use their GitHub profile URL instead (which I hope doesn't break anything that expects only an email address). Anyway, you should probably change that.
2. In the latest commit, you used the code I wrote and listed yourself as the author in the git metadata. I'm sure you didn't intend to mislead, but it's not accurate. Using, e.g. Magit, you can easily list another user as the author and yourself as the committer, as well as adding similar fields (C-c TAB is the keymap prefix). This is especially important for a package that's on GNU ELPA, which requires FSF copyright assignment: if I, as the maintainer, wrote the code, it's already handled; but if another contributor authored it, it has to be checked for assignment or cumulative contribution size, etc.

[pizzatorque]

Managed to change it :)

[ifinkelstein]

Here to say that I love the learning, positivity, and general tone of this exchange.

(Also loving activities.el)