kotct/packup ui #84

samontea · 2017-08-24T16:29:09Z

implemented kotct/packup from #66

performed manual user testing.
does not include unit testing

rye · 2017-08-24T23:51:43Z

This is a great start—thanks for your work so far on this, @samontea! 🎉 💯

Some off-hand notes from my initial evaluation, which aren't really criticisms of this work but more general ideas of where we can go from this:

After checking out and restarting Emacs, M-x kotct/packup fails with
```
command-execute: Autoloading file /Users/kristofer/Git/kotct/dot/.emacs.d/lisp/package/packup.elc failed to define function kotct/packup
```
when I switch branches, but this seems unrelated as deleting the .elc file for packup.el resolves this issue.
Can we add U and M to the help if they have been implemented? (It seems that they have been.)
When there are no updates to perform, can the packup buffer say something and not be blank? This could be a static idle thing defined in a variable.
I don't know how package-list from Emacs does column headers, but do we want to have column headers? Those are pretty cool. Also, can we separate the version with . rather than ? (Also, colors?)
After executing, a buffer pops up listing everything to be installed. As the installation process proceeds, this list buffer stays open while Emacs blocks for this installation. This is normal and probably part of the existing installation procedure. I'm wondering if we can do the heavy work in a way that doesn't bring that up or at least lets us customize. EDIT: I think this is part of the existing packup stuff, and has little to do with the UI.
Using this UI, obsolete packages are kept around—do we want to handle cleaning up for our users?

I will do an actual code review shortly, but again, this looks pretty awesome.

rye

Very good, just a couple of tweaks to be made I think.

rye · 2017-08-24T23:53:15Z

.emacs.d/lisp/package/packup.el

+(defvar kotct/packup-marker-char ?x
+  "In Packup, the current mark character.
+This is what the do-commands look for, and the flag the mark-commands store."
+  )


Move to previous line? I don't think we do this anywhere else.

rye · 2017-08-24T23:57:40Z

.emacs.d/lisp/package/packup.el

+Returns \"\" if there is no package name on the line."
+    (save-excursion
+      (beginning-of-line)
+      (let (p)


What is p? (Better name?)

i don't feel like it's ambiguous, and i don't feel like it deserves a better name. it's just a variable that's like not used and assigned two lines below to be a point. ¯_(ツ)_/¯

Okay so like would point be better?

rye · 2017-08-24T23:58:09Z

.emacs.d/lisp/package/packup.el

+             (while next-position
+               (goto-char next-position)
+               (setq results (cons ,body results))
+               (message "hi")


Remove "hi"?

rye · 2017-08-24T23:59:12Z

.emacs.d/lisp/package/packup.el

+
+(defun kotct/packup-help ()
+  (interactive)
+  (message "g-refresh m-mark u-unmark x-execute ?-help"))


Add U and M here?

we're going to end up with a lot of commands bound. if we document everything in the help it's going to be insane. i think we should just let people run describe-bindings. this is the way that dired does things.

That works. Then bind ? to describe-bindings, maybe?

uhm no that would be confusing to a new user. i think that we should do what dired does. if a user wants the keybindings described they can just run C-h b. we might investigate creating a control panel style help/command dialogue like magit has, but that's future work.

How about adding U and M, besides? dired still has a short help, and these would not be out of the scope of our short help.

rye · 2017-08-25T00:04:26Z

.emacs.d/lisp/package/packup.el

+  "Creates an interactive buffer to install/update packages."
+  (interactive)
+  (let ((old-buf (current-buffer))
+        (buffer (create-file-buffer "*packup*"))) ;; this is wrong...


Hmm, buffer has name packup for me with no cool asterisks. We should figure out how. Here is list-packages from package.el in Emacs, which seems to use get-buffer-create instead.

;;;###autoload (defun list-packages (&optional no-fetch) "Display a list of packages. This first fetches the updated list of packages before displaying, unless a prefix argument NO-FETCH is specified. The list is displayed in a buffer named `*Packages*'." (interactive "P") (require 'finder-inf nil t) ;; Initialize the package system if necessary. (unless package--initialized (package-initialize t)) ;; Integrate the package-menu with updating the archives. (add-hook 'package--post-download-archives-hook #'package-menu--post-refresh) (add-hook 'package--post-download-archives-hook #'package-menu--mark-or-notify-upgrades 'append) ;; Generate the Package Menu. (let ((buf (get-buffer-create "*Packages*"))) (with-current-buffer buf (package-menu-mode) ;; Fetch the remote list of packages. (unless no-fetch (package-menu-refresh)) ;; If we're not async, this would be redundant. (when package-menu-async (package-menu--generate nil t))) ;; The package menu buffer has keybindings. If the user types ;; `M-x list-packages', that suggests it should become current. (switch-to-buffer buf)))

uhm you're straight tripping if you run M-x kotct/packup it creates a buffer that is called *packup*. i'm like sure of that

i used create-file-buffer because it was what dired uses. i agree i should have used get-buffer-create. pushing that up now.

On Emacs HEAD it doesn't have asterisks, then, or they aren't displayed properly.

¯_(ツ)_/¯

samontea · 2017-08-25T02:25:22Z

Re: points you brought up in your comment, but not your code review.

I added a message letting the user know if there are now packages to be updated.
We don't receive header information.
The popping up buffer issue is out of scope for this issue. File a separate issue describing how we can remove that buffer when we execute a kotct/packup update script.
Cleaning up packages for the users is out of scope of this issue, and the general update feature. That is something we can discuss in a separate issue

rye

My only remaining problems are just a couple of leftovers from my previous review.

rye · 2017-08-25T15:30:39Z

.emacs.d/lisp/package/packup.el

@@ -47,7 +229,8 @@ If UPDATE is non-nil, out-of-date packages will be updated."
                   (terpri)
                   (princ (symbol-name (car package)))
                   (princ (if (cdr package) " (update)" " (install)"))))
-               (if (y-or-n-p "Auto install/update these package?")
+               (if (or auto-update
+                       (y-or-n-p "Auto install/update these package?"))


Auto install/update these package(s)?

This is part of existing code. Open an issue.

rye · 2017-08-25T15:32:47Z

.emacs.d/lisp/package/packup.el

+  "Creates an interactive buffer to install/update packages."
+  (interactive)
+  (let ((old-buf (current-buffer))
+        (buffer (get-buffer-create "*packup*")))


I think the display problems are due to capitalization. Can this be capitalized?

Actually, never mind. It's not. It seems to be a problem with the mode.

No, I revise this again. It's because you set mode-line-buffer-identification to packup that we get rid of the bolded name.

EDIT: Added additional review for this.

rye · 2017-08-25T15:33:29Z

.emacs.d/lisp/package/packup.el

+
+(defun kotct/packup-help ()
+  (interactive)
+  (message "g-refresh m-mark u-unmark x-execute ?-help"))


How about adding U and M, besides? dired still has a short help, and these would not be out of the scope of our short help.

rye · 2017-08-25T15:45:14Z

.emacs.d/lisp/package/packup.el

+  (setq major-mode 'packup-mode
+        mode-name "Packup"
+        buffer-read-only t ;; read only
+        mode-line-buffer-identification "packup")


Can this be removed? I like the default buffer ID, and it's less confusing.

rye

This looks good enough to me now.

This is in accordance with our code style.

cg505 · 2018-06-22T04:34:41Z

yooooooooo

samontea requested review from rye and cg505 August 24, 2017 16:29

rye requested changes Aug 25, 2017

View reviewed changes

rye approved these changes Aug 29, 2017

View reviewed changes

rye closed this Nov 5, 2017

rye reopened this Nov 5, 2017

cg505 mentioned this pull request May 30, 2018

packup does not respect package-archive-priorities #99

Closed

samontea force-pushed the packup-ui branch from 1f00f9d to d10d4a2 Compare June 21, 2018 20:25

samontea and others added 13 commits June 21, 2018 19:21

add kotct/packup which initializes buffer for ui

fd1d53f

add commands mark & unmark to dired (w/ region support!)

e2e5b25

change format for packup

ff933ff

update documentation, fix typos

939d964

implement kotct/packup-{do-update,refresh}

7aa6962

add kotct/packup-{mark-all,unmark-all,help}

afb616f

switch kotct/packup to get-buffer-create

a4377f7

cleanup 🚿

766944e

alert user if no new packages available

8c702a6

small cleanup 🚿

286f54c

remove buffer identification

66b0400

Add autoload comment at top of packup.el

30f3d67

This is in accordance with our code style.

Fix English used in auto install/update process

a0e815a

samontea force-pushed the packup-ui branch from d10d4a2 to a0e815a Compare June 21, 2018 23:21

samontea merged commit 4e01807 into master Jun 21, 2018

samontea deleted the packup-ui branch June 21, 2018 23:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kotct/packup ui #84

kotct/packup ui #84

samontea commented Aug 24, 2017

rye commented Aug 24, 2017 •

edited

Loading

rye left a comment

rye Aug 24, 2017

rye Aug 24, 2017

samontea Aug 25, 2017

rye Aug 25, 2017

samontea Aug 25, 2017

rye Aug 24, 2017

rye Aug 24, 2017

samontea Aug 25, 2017

rye Aug 25, 2017

samontea Aug 25, 2017

rye Aug 25, 2017

rye Aug 25, 2017

samontea Aug 25, 2017

rye Aug 25, 2017

samontea Aug 25, 2017

samontea commented Aug 25, 2017

rye left a comment

rye Aug 25, 2017

samontea Aug 27, 2017

rye Aug 25, 2017

rye Aug 25, 2017

rye Aug 25, 2017 •

edited

Loading

rye Aug 25, 2017

rye Aug 25, 2017

rye left a comment

cg505 commented Jun 22, 2018

kotct/packup ui #84

kotct/packup ui #84

Conversation

samontea commented Aug 24, 2017

rye commented Aug 24, 2017 • edited Loading

rye 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

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

samontea commented Aug 25, 2017

rye 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

rye Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rye left a comment

Choose a reason for hiding this comment

cg505 commented Jun 22, 2018

rye commented Aug 24, 2017 •

edited

Loading

rye Aug 25, 2017 •

edited

Loading