[PATCH] Show avatars from Libravatar.

  • Open
  • quality assurance status badge
Details
3 participants
  • Arun Isaac
  • Felix Lechner
  • noe
Owner
unassigned
Submitted by
Felix Lechner
Severity
normal
F
F
Felix Lechner wrote on 11 May 05:20 +0200
(address . bug-mumi@patchwise.org)(name . Felix Lechner)(address . felix.lechner@lease-up.com)
20240511032008.16270-1-felix.lechner@lease-up.com
Requires guile-avatar from here:


The packaging code for guile-avatar can be added to Guix by accepting
this patch:


In Guix, the package should then be mentioned as an "input" to mumi,
together with an updated commit hash or version for mumi.

A live preview of this change is available at:

---
mumi/web/view/html.scm | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)

Toggle diff (54 lines)
diff --git a/mumi/web/view/html.scm b/mumi/web/view/html.scm
index 2275799..bd5caec 100644
--- a/mumi/web/view/html.scm
+++ b/mumi/web/view/html.scm
@@ -19,6 +19,7 @@
;;; <http://www.gnu.org/licenses/>.
(define-module (mumi web view html)
+ #:use-module (avatar url)
#:use-module (email email)
#:use-module (mumi config)
#:use-module (mumi debbugs)
@@ -451,12 +452,12 @@ failed to process associated messages.")
,(map (match-lambda
((message-number message)
`(li
- (div
- (@ (class "avatar")
- (style ,(string-append "background-color:"
- (avatar-color (sender-email message)
- (map extract-email parties)))))
- ,(string-upcase (string-take (sender-name message) 1)))
+ (img (@ (class "avatar")
+ (style ,(string-append "background-color:"
+ (avatar-color (sender-email message)
+ (map extract-email parties))))
+ (src ,(libravatar-url (sender-email message) #:default "404"))
+ (alt ,(string-upcase (string-take (sender-name message) 1)))))
(span (@ (class "date"))
(a (@ (href ,(string-append "#" (number->string
message-number))))
@@ -621,12 +622,12 @@ currently disabled."))
(id ,(number->string message-number))))
(a (@ (class "message-anchor")
(id ,(format #false "msgid-~a" (msgid-hash (message-id message))))))
- (div
- (@ (class "avatar")
- (style ,(string-append "background-color:"
- (avatar-color (sender-email message)
- (map extract-email parties)))))
- ,(string-upcase (string-take (sender-name message) 1)))
+ (img (@ (class "avatar")
+ (style ,(string-append "background-color:"
+ (avatar-color (sender-email message)
+ (map extract-email parties))))
+ (src ,(libravatar-url (sender-email message) #:default "404"))
+ (alt ,(string-upcase (string-take (sender-name message) 1)))))
(article
(@ (class "message"))
(header

base-commit: 394c90d4a176756b9f0f4a716a2646ab98d8f167
--
2.41.0
F
F
Felix Lechner wrote on 11 May 05:23 +0200
(no subject)
(address . control@debbugs.gnu.org)
875xvlxc8y.fsf@lease-up.com
reassign 70871 mumi
thanks
A
A
Arun Isaac wrote on 11 May 18:53 +0200
Re: bug#70871: [PATCH] Show avatars from Libravatar.
(name . Felix Lechner)(address . felix.lechner@lease-up.com)(address . 70871@debbugs.gnu.org)
87plts5lyy.fsf@systemreboot.net
Hi Felix,

Libravatar for mumi is a cool idea. I am happy to merge this patch with
the suggested changes.

There seems to be one more place (a total of three places) in
mumi/web/view/html.scm where there is an avatar. Search for
"string-upcase" in the code. Could you change that to Libravatar as
well? And, while you are at it, would you mind deduplicating these three
avatars into a single function or similar?

Also, we now need to check for guile-avatar in configure.ac.

BTW, what is patchwise.org? Did I miss some conversation about it? I was
under the impression that we should be sending to
bug-mumi@gnu.org. Thank you for the clarification!

Regards,
Arun
A
A
Arun Isaac wrote on 13 May 01:51 +0200
(name . Felix Lechner)(address . felix.lechner@lease-up.com)(address . 70871@debbugs.gnu.org)
87a5ku4mhy.fsf@systemreboot.net
Hi Felix,

Could you also make the deduplication and the switch to libravatar two
separate patches? Thanks!

I also just remembered that the alt attribute of the img tag is meant to
be used in place of the image (visually challenged readers using a
screen reader, image download failed on a slow connection, etc.). So, we
shouldn't use the alt attribute as a fallback for users without
avatars. We should do it some other way.

Regards,
Arun
F
F
Felix Lechner wrote on 13 May 21:30 +0200
(name . Arun Isaac)(address . arunisaac@systemreboot.net)(address . 70871@patchwise.org)
87pltpwlvp.fsf@lease-up.com
Hi Arun,

On Mon, May 13 2024, Arun Isaac wrote:

Toggle quote (3 lines)
> would you mind deduplicating these three avatars into a single
> function or similar?

Personally, I do not see the benefit of that abstraction for HTML. For
one, the code is a lot easier to parse next to a Web Inspector that
shows the same tags.

There is also not much to abstract: It's a single 'img' tag.

Moreover, the avatars are also in different positions. They do
different things on the page. Most significantly, they are subject to
different CSS.

You'll see why that matters just below.

Toggle quote (3 lines)
> There seems to be one more place in mumi/web/view/html.scm where there
> is an avatar.

There is no third avatar. If you haven't yet followed the CSS with a
Web Inspector, please look for the 'display: none' for that 'img' tag.
It comes from here:


Does the "third" avatar more or less duplicate with the big one in
front? Do you plan to use the third avatar? I'd drop it.

Toggle quote (6 lines)
> I also just remembered that the alt attribute of the img tag is meant to
> be used in place of the image (visually challenged readers using a
> screen reader, image download failed on a slow connection, etc.). So, we
> shouldn't use the alt attribute as a fallback for users without
> avatars.

Would you please explain that rationale? A blind person who disables
the loading of images would still "see" (or perhaps hear) the 'alt'
character. How does my code break anything for a blind person, please?

Toggle quote (2 lines)
> Also, we now need to check for guile-avatar in configure.ac.

It is not necessary for my deployment on patchwise.org, which already
shows the avatars.

For an interpreted language like Guile there is no need to provide
prerequisite checks at configure time unless they enable workarounds to
build without. The Guile module detections currently found in
configure.ac are luxuries. I would remove them.

If you prefer to keep them and add guile-avatar for the sake of
consistency, please feel free to amend the patch. I do not insist on Git
authorship for my contributions. I am sorry, but I am super busy.

Toggle quote (2 lines)
> what is patchwise.org? Did I miss some conversation about it?

That's the domain I hope to use in the near future---and in cooperation
with the FSF---to show off my plans for a Debbugs upgrade based on Mumi.

FSF declined to upgrade debbugs.gnu.org to the latest Debbugs version
(from Debian) because my packaging was based on Guix. FSF wants Trisquel
plus Ansible.

FSF encouraged me to complete my work elsewhere and demonstrate it when
done. It was not my first choice, please believe me. I protested
loudly in various places. Either way, Patchwise is licensed under GNU
Affero. There are no hidden goals.

Toggle quote (3 lines)
> I was under the impression that we should be sending to
> bug-mumi@gnu.org.

I had hoped to open the bug by bouncing the message from patchwise.org
to gnu.org. The Patchwise bouncer works in all other cases, such as
when amending or controlling bugs (please try it, or look at the headers
for this message!) or access to the mailing lists. I'll have to talk to
FSF about using the envelope address for submissions, as well.

The Patchwise bouncer is essential for the automatic sequencer that will
allow folks to submit a patch series in one go. I think your command
line tool may do something similar.

Also, the patchwise.org effort has not been made public. You are one of
the first people to hear about it.

I'd be happy to cooperate, if you agree to be less picky about my
patches.

Kind regards
Felix
A
A
Arun Isaac wrote on 19 May 04:50 +0200
(name . Felix Lechner)(address . felix.lechner@lease-up.com)
87ttiuikgx.fsf@systemreboot.net
Hi Felix,

Toggle quote (2 lines)
> There is also not much to abstract: It's a single 'img' tag.

Well, it's repeated in three places. So, it's probably worth abstracting
out.

Toggle quote (4 lines)
> There is no third avatar. If you haven't yet followed the CSS with a
> Web Inspector, please look for the 'display: none' for that 'img' tag.
> It comes from here:

I believe the third avatar shows up on smaller screens. Try using the
"responsive mode" (Ctrl+Shift+m) in Icecat to simulate a smaller screen.

Toggle quote (11 lines)
>> I also just remembered that the alt attribute of the img tag is meant to
>> be used in place of the image (visually challenged readers using a
>> screen reader, image download failed on a slow connection, etc.). So, we
>> shouldn't use the alt attribute as a fallback for users without
>> avatars.
>
> Would you please explain that rationale? A blind person who disables
> the loading of images would still "see" (or perhaps hear) the 'alt'
> character. How does my code break anything for a blind person,
> please?

alt text of "A" for "Arun Isaac" would not make sense if read out aloud
by a screen reader. It should rather be "Profile picture of Arun Isaac"
or similar. Perhaps, it should also be marked up with
aria-hidden=true. See

I'm not saying the current situation in mumi gets accessibility
right. But, while we are changing things, we might as well get things
right.

Toggle quote (5 lines)
> For an interpreted language like Guile there is no need to provide
> prerequisite checks at configure time unless they enable workarounds to
> build without. The Guile module detections currently found in
> configure.ac are luxuries. I would remove them.

I agree. But, I'm mainly trying to remain consistent with the existing
code base.

Toggle quote (3 lines)
> If you prefer to keep them and add guile-avatar for the sake of
> consistency, please feel free to amend the patch.

I am happy to make this change myself.

Toggle quote (2 lines)
> I am sorry, but I am super busy.

Sorry, so am I, and so is everyone else hacking away on Guix and mumi in
their spare time.

Toggle quote (3 lines)
> I'd be happy to cooperate, if you agree to be less picky about my
> patches.

As a reviewer, it is kinda my job to bring contributions up to shape
before they get applied. I am happy to work in good faith towards that
goal. And, I don't feel I have been particularly picky in my review. The
changes I proposed were pretty reasonable.

Regards,
Arun
N
N
noe wrote on 2 Nov 16:34 +0100
[PATCH v2 0/2] Show avatars from Libavatar.
(address . 70871@debbugs.gnu.org)(name . Noé Lopez)(address . noelopez@free.fr)
20241102153450.958-1-noe@xn--no-cja.eu
From: Noé Lopez <noelopez@free.fr>

Hi,

Here is an updated version of the patch with the requested changes.

I agree with the changes requested because it is that rigor that keeps
the code easy to understand and modify.

The test data does not have libavatar associated emails, but you can
test it by replacing « user » with another email (mine should work):

find . -not -path '*/\.git/*' -type f -exec sed -i 's/user@example.com/REPLACEME/g' '{}' \;

Good afternoon,
Noé

Noé Lopez (2):
web: Extract user avatars to function.
web: Show avatars from Libravatar.

configure.ac | 5 +++++
mumi/web/view/html.scm | 36 ++++++++++++++++++------------------
2 files changed, 23 insertions(+), 18 deletions(-)


base-commit: d8ff427dc16f44f4b15684207d3c531301fb0407
--
2.46.0
N
N
noe wrote on 2 Nov 16:34 +0100
[PATCH v2 1/2] web: Extract user avatars to function.
(address . 70871@debbugs.gnu.org)(name . Noé Lopez)(address . noelopez@free.fr)
20241102153450.958-2-noe@xn--no-cja.eu
From: Noé Lopez <noelopez@free.fr>

* mumi/web/view/html.scm (avatar): New function.
---
mumi/web/view/html.scm | 31 +++++++++++++------------------
1 file changed, 13 insertions(+), 18 deletions(-)

Toggle diff (63 lines)
diff --git a/mumi/web/view/html.scm b/mumi/web/view/html.scm
index fa19ab4..661f248 100644
--- a/mumi/web/view/html.scm
+++ b/mumi/web/view/html.scm
@@ -424,6 +424,16 @@ failed to process associated messages.")
"Encode PARTS and join them together into an absolute URI path."
(string-append "/" (encode-and-join-uri-path parts)))
+(define* (avatar sender
+ #:optional
+ (participants (list (sender-email sender))))
+ `(div (@ (class "avatar")
+ (style ,(string-append
+ "background-color:"
+ (avatar-color (sender-email sender)
+ participants))))
+ ,(string-upcase (string-take (sender-name sender) 1))))
+
(define* (issue-page bug #:key flash-message plain?)
"Render the conversation for the given BUG."
(define id (bug-num bug))
@@ -450,12 +460,7 @@ failed to process associated messages.")
,(map (match-lambda
((message-number message)
`(li
- (div
- (@ (class "avatar")
- (style ,(string-append "background-color:"
- (avatar-color (sender-email message)
- (map extract-email parties)))))
- ,(string-upcase (string-take (sender-name message) 1)))
+ ,(avatar message (map extract-email parties))
(span (@ (class "date"))
(a (@ (href ,(string-append "#" (number->string
message-number))))
@@ -638,23 +643,13 @@ currently disabled."))
(id ,(number->string message-number))))
(a (@ (class "message-anchor")
(id ,(format #false "msgid-~a" (msgid-hash (message-id message))))))
- (div
- (@ (class "avatar")
- (style ,(string-append "background-color:"
- (avatar-color (sender-email message)
- (map extract-email parties)))))
- ,(string-upcase (string-take (sender-name message) 1)))
+ ,(avatar message (map extract-email parties))
(article
(@ (class "message"))
(header
(div
(@ (class "from"))
- (div
- (@ (class "avatar")
- (style ,(string-append "background-color:"
- (avatar-color (sender-email message)
- (map extract-email parties)))))
- ,(string-upcase (string-take (sender-name message) 1)))
+ ,(avatar message (map extract-email parties))
(span (@ (class "address")) ,(sender-name message))
" wrote "
(span (@ (class "date"))
--
2.46.0
N
N
noe wrote on 2 Nov 16:34 +0100
[PATCH v2 2/2] web: Show avatars from Libravatar.
(address . 70871@debbugs.gnu.org)(name . Noé Lopez)(address . noelopez@free.fr)
20241102153450.958-3-noe@xn--no-cja.eu
From: Noé Lopez <noelopez@free.fr>

* configure.ac: New dependency.
* mumi/web/view/html.scm: Use Libavatar images as avatars.
---
configure.ac | 5 +++++
mumi/web/view/html.scm | 11 ++++++++---
2 files changed, 13 insertions(+), 3 deletions(-)

Toggle diff (50 lines)
diff --git a/configure.ac b/configure.ac
index 5981682..ff754b4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -74,6 +74,11 @@ if test "x$have_kolam" != "xyes"; then
AC_MSG_ERROR([Guile kolam is missing; please install it.])
fi
+GUILE_MODULE_AVAILABLE([have_avatar], [(avatar url)])
+if test "x$have_avatar" != "xyes"; then
+ AC_MSG_ERROR([Guile avatar is missing; please install it.])
+fi
+
guilemoduledir="${datarootdir}/guile/site/${GUILE_EFFECTIVE_VERSION}"
AC_SUBST([guilemoduledir])
AC_SUBST([GUILE_EFFECTIVE_VERSION])
diff --git a/mumi/web/view/html.scm b/mumi/web/view/html.scm
index 661f248..a037a74 100644
--- a/mumi/web/view/html.scm
+++ b/mumi/web/view/html.scm
@@ -19,6 +19,7 @@
;;; <http://www.gnu.org/licenses/>.
(define-module (mumi web view html)
+ #:use-module (avatar url)
#:use-module (email email)
#:use-module (mumi config)
#:use-module (mumi debbugs)
@@ -427,12 +428,16 @@ failed to process associated messages.")
(define* (avatar sender
#:optional
(participants (list (sender-email sender))))
- `(div (@ (class "avatar")
+ `(img (@ (class "avatar")
(style ,(string-append
"background-color:"
(avatar-color (sender-email sender)
- participants))))
- ,(string-upcase (string-take (sender-name sender) 1))))
+ participants)))
+ (src ,(libravatar-url (sender-email sender)
+ #:default "404"
+ #:height 48))
+ (alt ,(string-upcase (string-take (sender-name sender) 1)))
+ (aria-hidden "true"))))
(define* (issue-page bug #:key flash-message plain?)
"Render the conversation for the given BUG."
--
2.46.0
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 70871@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 70871
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch