New UI #63

tanrax · 2025-10-13T07:19:04Z

Would that be interesting?

basil-conto

Thank you very much for the contribution! There is definitely interest in improving the UI.

I won't have time to review this carefully for a little while, so here are some thoughts after a quick scan of the changes.

The topic of improving the UI has been brought up before. Have you looked over #44, #45, and #55? Since those discussions, I have also learned of another attractive UI/MVC framework that is built into Emacs, namely ewoc: (info "(elisp) Abstract Display")
Ideally the choice of UI should be configurable via a user option. Similarly, we should try to avoid changing default key bindings (and their effects) if possible.

basil-conto · 2025-10-13T10:26:17Z

hackernews.el

+      (when size
+        (push `(:height ,size) props))
+      (when font-color
+        (push `(:foreground ,font-color) props))
+      (when background-color
+        (push `(:background ,background-color) props))
+      (when props
+        (put-text-property start end 'face (apply #'append props))))))


It would be more configurable and play better with Font Lock if we used named faces defined via defface rather than ad-hoc anonymous faces. Ideally the default colours would adapt well to the user's background mode etc.

basil-conto · 2025-10-13T10:30:49Z

hackernews.el

+                 :notify (lambda (&rest _)
+                           (hackernews-top-stories))
+                 :help-echo "View top stories"
+                 " 🔥 Top ")


I don't think we can assume that emoji will always be displayable in the user's system/font, so this should degrade gracefully or at least be configurable. IIRC more recent Emacs versions introduced a mechanism for this, namely define-icon or similar: (info "(elisp) Icons")

basil-conto · 2025-10-13T10:33:07Z

hackernews.el

+  (let ((separator-regex (concat "^" (regexp-quote (hackernews--string-separator)) "$")))
+    (if (search-forward-regexp separator-regex nil t)
+        (forward-line 2)


My first reaction is that this is too tightly coupled with the display format. I would prefer to avoid search-based motion, instead relying on either some feature of the UI, or some marker (e.g. text property) that we place in the buffer that is independent of how the buffer is displayed.

basil-conto · 2025-10-13T10:37:33Z

hackernews.el

-         (inhibit-read-only t))
+         (feed  (hackernews--get :feed))
+         (feed-name (hackernews--feed-name feed))
+         (is-first-load (= (point-max) 1)))


In general there's no need to compare against concrete position values in Elisp. If you want to check whether the narrowed portion of the buffer is empty, you can compare point-min to point-max; and for checking whether the buffer is empty irrespective of narrowing, there is buffer-size.

basil-conto · 2025-10-13T10:38:20Z

hackernews.el

+         (is-first-load (= (point-max) 1)))
+
+    ;; Temporarily disable read-only mode
+    (read-only-mode -1)


Why not bind inhibit-read-only?

basil-conto · 2025-10-13T10:46:29Z

hackernews.el

+    ;; Disable line numbers
+    (when (fboundp 'display-line-numbers-mode)
+      (display-line-numbers-mode 0))


Why is this needed? Ideally we shouldn't have to override user preferences.

basil-conto · 2025-10-13T10:48:55Z

hackernews.el

+  (when (and hackernews-enable-visual-fill-column
+             (require 'visual-fill-column nil t))
+    (setq-local visual-fill-column-center-text t)
+    (setq-local visual-fill-column-width hackernews-display-width)
+    (visual-fill-column-mode 1))


Why does hackernews need to be aware of visual-fill-column-mode, and load and enable it explicitly? Can't the user enable the minor mode as desired via hackernews-mode-hook? If the setup is nontrivial we can always document it in our README/wiki.

basil-conto · 2025-10-13T10:49:41Z

hackernews.el

+  ;; Activate the keymap
+  (use-local-map hackernews-mode-map))


Why is this needed? Usually this is done automatically by define-derived-mode.

basil-conto · 2025-10-13T10:50:21Z

hackernews.el

+        ;; Kill local variables BEFORE disabling read-only (like Lobsters)
+        (kill-all-local-variables)


Why is this needed?

basil-conto · 2025-10-13T10:50:40Z

hackernews.el

+        ;; Temporarily disable read-only mode
+        (read-only-mode -1)
+
+        ;; Clear buffer
        (let ((inhibit-read-only t))


Why is this needed given the subsequent inhibit-read-only?

basil-conto · 2025-10-13T10:57:53Z

hackernews.el

 ;; Keywords: comm hypermedia news
-;; Version: 0.7.1
+;; Version: 0.8.0
+;; Package-Requires: ((emacs "24.3"))


Either:

remove the versions lower than 24.3 from .github/workflows/build.yml; or

add a dependency on the cl-lib compatibility package from GNU ELPA: https://elpa.gnu.org/packages/cl-lib.html

Personally I am happy to raise the minimum required Emacs version as needed, given that the hackernews package talks to remote third-party machines over the network.

tanrax · 2025-10-13T12:00:10Z

Thank you very much for the detailed review and feedback!

Previous discussions and ewoc: I'll review those now. The ewoc framework sounds interesting; I wasn't familiar with it.
Configurable UI: The default could remain the current minimal UI, and users could opt into the enhanced
version. Does this sound like the right approach?

Regarding specific code issues:

Anonymous faces vs defface: You're completely right. I'll refactor this.
Emoji support: Good point. It should be configurable.
Search-based navigation: I see the issue with coupling navigation to the visual format. Would you recommend using text properties or markers to identify item boundaries?
Position comparisons: I'll change (= (point-max) 1) to (= (buffer-size) 0) or (= (point-min) (point-max)).
The inhibit-read-only, switch-to-buffer, mode timing, and other issues: These were carryovers from my attempts to get the code working. I will make changes.

Well...would you prefer that I:

Make all these fixes in the current PR, or
Close this PR and open a new one after studying ewoc and the previous discussions more carefully?

basil-conto · 2025-10-13T14:05:46Z

Configurable UI: The default could remain the current minimal UI, and users could opt into the enhanced version. Does this sound like the right approach?

That would be amazing for backward compatibility, which though a general goal of the Emacs project, may or may not be our highest priority for the hackernews project. However, a secondary benefit of designing the new UI to be optional is that the resulting solution will hopefully be general enough to afford any number of future UIs :).

If it proves too much of a hassle we can always reconsider.

* Search-based navigation: I see the issue with coupling navigation to the visual format. Would you recommend using text properties or markers to identify item boundaries?

Markers can degrade Emacs redisplay performance (at least this was the case historically in Emacs, though more recent changes to Emacs internals may have mitigated this). The penalty may be negligible in the case of hackernews, but if all else is the same, then my vote is for plain text properties.

However, if the Widget/EWOC/etc. library provides functions for locating UI elements, then I think we should prefer that over a custom solution.

Well...would you prefer that I:

1. Make all these fixes in the current PR, or

2. Close this PR and open a new one after studying ewoc and the previous discussions more carefully?

It's completely up to you, AFAIC there's no harm in leaving this PR open and in progress.

Thanks again, and happy hacking! :)

tanrax added 3 commits October 13, 2025 08:52

Added new UI

12587ef

Updated documentation

bf38d27

Moved comments button

5bcdebe

basil-conto added the enhancement Feature request label Oct 13, 2025

basil-conto reviewed Oct 13, 2025

View reviewed changes

Added optional emojis

36110c6

		;; Kill local variables BEFORE disabling read-only (like Lobsters)
		(kill-all-local-variables)

New UI #63

Are you sure you want to change the base?

New UI #63

Uh oh!

Conversation

tanrax commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

basil-conto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanrax commented Oct 13, 2025

Uh oh!

basil-conto commented Oct 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tanrax commented Oct 13, 2025 •

edited

Loading