Closed basil-conto closed 6 years ago
I like the idea.
If no one is working on this then I'd be willing to take it on.
It would be lovely if you did @accidentalrebel!! Thank you! \o/
I might not be able to work on this anytime soon. If anyone wants to take this on then please do!
No worries @accidentalrebel, thanks for the interest and letting us know!
I've never contributed to an Emacs package before, but if you'd be kind enough to guide me I'd like to take a crack at it.
As far as I can see this requires three components:
customize
field in the .emacs
file is impolite.)I'm looking for an answer to question 2. I think I have an idea of how to do the rest, provided I can ask questions if I get stuck.
I've never contributed to an Emacs package before, but if you'd be kind enough to guide me I'd like to take a crack at it.
Thanks for your interest! I'll gladly help however I can, but how useful that will be remains to be seen. :)
What is the standard place to save small user-facing lists?
Anywhere under user-emacs-directory
is fine, so long as the file name is customisable via a user option (defcustom
); see (elisp) Standard File Names
. I recommend littering user-emacs-directory
as little as possible by default, e.g. by placing the "database" under a hidden directory such as ~/.emacs.d/.hackernews/visited
. See also the commentary of https://github.com/emacscollective/no-littering.
I'm guessing that making a database is overkill
I think so. A quick test shows that writing a list of a million fixnums to a file takes ~0.5s and 6.7MB, and reading it back takes ~0.2s on my system. It would, nevertheless, be nice to look into which, if any, built-in packages and data structures Emacs provides which could help with this. For example, it may turn out to be more efficient to read/write a literal hash-table or obarray. We should also consider which attributes we need to store along with the visited ID, such as date visited, which could eventually allow, say, purging old entries. It'd also be nice to look into the feasibility/usefulness of automatic compression/decompression of this file, which Emacs should support quite well. None of this has to be implemented up-front, of course, but the more we design up-front the fewer headaches we'll have later on.
adding a
customize
field in the.emacs
file is impolite
To put it politely, yes. :)
provided I can ask questions if I get stuck.
Of course! Feel free to email me directly as you see fit, too.
Thanks again, and happy hacking!
Ooh, just slapping a .gz
onto the end of the file name automagically decreases file size (and increases writing time both) by ~3x; I wonder if this is also supported on Windows.
Great. As a prototype I'll just serialize a hashset into <user-emacs-directory>/hackernews.cache
.
Branch in progress: https://github.com/clarete/hackernews.el/compare/master...matthew-piziak:visited-links
Did a quick ten-minute prototype. New face, new button type, in-memory variable, no deduplication, requires refresh for links to change. But it looks nice.
But it looks nice.
It does indeed. I'm wondering if it's high time we changed the default definition of the hackernews-comment-count
face, though, to inherit from link
instead of link-visited
, so that it doesn't clash with this feature. @clarete Any objections?
@basil-conto What's weird is that in the code it already does.
(defface hackernews-comment-count
'((t :inherit hackernews-link))
"Face used for comment counts."
:package-version '(hackernews . "0.4.0")
:group 'hackernews)
But in the customize-face menu it inherits from link-visited
. I'm reading something wrong...
We should probably distinguish the comment link from the article link somehow. And we should probably also make them separately readable. So we'd have two hashsets: one for the article IDs where the article link was clicked, and one for the comment IDs where the comment link was clicked.
What's weird is that in the code it already does.
Oops, right you are, of course. Except that it's not weird at all, but rather expected behaviour. It's just that I completely forgot I'm to blame for these customisations in the zenburn
custom theme: https://github.com/bbatsov/zenburn-emacs/pull/284 :)
I no longer use zenburn
, but I have a similar customisation in my personal custom theme.
But in the customize-face menu it inherits from
link-visited
. I'm reading something wrong...
I presume you also use a custom theme which applies this customisation.
So, false alarm @clarete. Sorry about the noise.
We should probably distinguish the comment link from the article link somehow.
Perhaps, which is why I felt compelled to have the former inherit from link
and the latter from link-visited
. But any customisation you do beyond that has to handle different terminal classes well; see (elisp) Defining Faces
. If you're willing to do this, then by all means, feel free (I don't feel confident with colours/styles/etc.). But it's not high priority, both because there's nothing currently stopping users from painting the faces whatever they like, and because we shouldn't assume that we know better than the user what they like by default. At the end of the day, both button types are links, so there's nothing wrong with fontifying them both as such.
And we should probably also make them separately readable. So we'd have two hashsets: one for the article IDs where the article link was clicked, and one for the comment IDs where the comment link was clicked.
This raises the question of how we define uniqueness. Would we ever care about keeping track of specific URLs that have been visited, rather than their corresponding Firebase IDs? Say, for example, I visit a post's comments page today, and Hacker News changes its URL format tomorrow. Do we agree that the new comments page URL should still be marked as visited, based on its ID?
Off the top of my head, I think a clean way of associating each type of link with its corresponding ID set would be via a custom button property. For example, the hackernews-link
button type could be given the property, say, hackernews-ids
, whose value is the set of visited item IDs, or that variable's symbol name. Then the rendering and button action code would just look up and update each button type's associated set. This is all very hypothetical, but WDYT?
At the end of the day, both button types are links, so there's nothing wrong with fontifying them both as such.
That's very sensible. I'll leave them alone in that case. I'm glad that I'm thinking aloud before forging ahead; you're giving me very useful feedback!
Say, for example, I visit a post's comments page today, and Hacker News changes its URL format tomorrow. Do we agree that the new comments page URL should still be marked as visited, based on its ID?
I think I know what you're getting at, but I'll prototype something and let you review. Sometimes it's easier to let code speak for itself.
Okay, so here's a hack:
Note that the comments and article links are now readable separately. Unfortunately this depends on the gross logic of checking for the "ycombinator" string in the link!
I've duplicated some faces here. Take a look. If this is on the right track then I'll add a proper "article-or-comment-link" marker to the button so that the URL hack isn't necessary.
https://github.com/clarete/hackernews.el/compare/master...matthew-piziak:visited-links
Thanks @matthew-piziak, looks good and on the right track. The UI code can easily be generalised and made more agnostic later on.
My only big design question is whether we want to support font-lock-mode
, which is enabled globally by default. Here's a breakdown of my concerns, using the hackernews-link
button type as an example:
hackernews-link
and hackernews-link-visited
are redundant:
hackernews--visited-article-ids
.hackernews-link
and hackernews-link-visited
differ only in their face
.(button-put (point) 'face 'some-face)
(button-put (point) 'type 'some-button-type)
face
.font-lock-mode
doesn't allow in-place modification of the face
text property; see https://emacs.stackexchange.com/a/17143/15748.So, the simplest solution in terms of both design and implementation is to have a single button type for each link type, as per (3)(i). Since we have two link types (articles and comments), we only need two button types (hackernews-link
and hackernews-comment-count
, respectively). Since we already have these two button types, the only thing that needs to be changed is to specify another "visited" face for each button type, e.g.
(define-button-type 'hackernews-link
'action #'hackernews-browse-url-action
'face 'hackernews-link
'follow-link t
'hackernews-vface 'hackernews-link-visited ; Visited face
'keymap hackernews-button-map)
Then any button press actions just need to change that button instance's face
to the value of their button type's hackernews-vface
property.
BUT, given (4), we can't do this - font-lock-mode
simply ignores any face
property changes, and the button instance's fontification stays the same.
The simplest (because I know next to nothing about font-lock-mode
) workaround is to locally disable font-lock-mode
in hackernews
buffers. Judging from the introduction in (elisp) Font Lock Mode
, this should be kosher because hackernews-mode
is a special-mode
and doesn't have to handle any text syntax in its buffers. But (again, because I know next to nothing about font-lock-mode
) I can't guarantee that disabling font-lock-mode
in hackernews
buffers won't break any other Emacs features. I hope and assume not.
The other solution is to continue supporting font-lock-mode
by defining "normal" and "visited" variations of each button type, as per (3)(ii). This means we never have to worry about whether faces are being applied properly, at the expense of more complicated button type hierarchies and button press action logic.
I'm kinda torn between the two approaches, but am leaning towards the latter, both because it's closer to vanilla emacs -Q
and because I'm far more familiar with button.el
than font-lock.el
. WDYT?
Either way, I think it'd be cleaner to couple the visited ID sets with their button type, rather than having each set as a separate variable. For example, you could write the following:
(define-button-type 'hackernews-link
'action #'hackernews-browse-url-action
'face 'hackernews-link
'follow-link t
'hackernews-ids () ; Visited IDs
'hackernews-visited 'hackernews-link-visited ; Visited subtype
'keymap hackernews-button-map)
(define-button-type 'hackernews-link-visited
'face 'hackernews-link-visited
'supertype 'hackernews-link)
and then remove hackernews--visited-article-ids
altogether.
Okay, great! I've opted for keeping font-lock-mode
, and associating the IDs with the buttons seems to be working. Unfortunately it still requires a redraw. What's the best way to redraw the buttons without refreshing the feed?
https://github.com/clarete/hackernews.el/compare/master...matthew-piziak:visited-links
What's the best way to redraw the buttons without refreshing the feed?
Emacs' sophisticated (read: complicated) redisplay code takes care of that; all you have to do is change the button's type in-place, e.g. (button-put (point) 'type 'hackernews-link-visited)
.
all you have to do is change the button's type in-place
If you get a read-only error just bind inhibit-read-only
around button-put
. And take all of my 100% untested suggestions with a grain of salt because I'm AFK and writing from memory. :)
Oh wow, that works great. This looks like it's totally working except for persistence across sessions.
Please check whether the simple checklist I added to this issue is accurate/reasonable.
@basil-conto Yep, that all looks good. I'll proceed along those lines. :)
Also, feel free to open a PR, even a "[WIP]" one, whenever you're comfortable with it.
I've added a defcustom
for hackernews-show-visited
, so you can check that box. :)
Thanks! (I think you can also tick the boxes if you prefer. [Edit - actually you can't (yet).])
Here's the PR: https://github.com/clarete/hackernews.el/pull/48
Thanks for walking me through it!
Thanks for working on it!
It would be nice to:
link
/link-visited
.TODO