GNU bug report logs - #79559
[PATCH] Refactor preview code

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 79559 in the body.
You can then email your comments to 79559 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Al Haji-Ali <abdo.haji.ali <at> gmail.com>
To: bug-auctex <at> gnu.org
Cc: Ikumi Keita <ikumi <at> ikumi.que.jp>, Arash Esbati <arash <at> gnu.org>
Subject: [PATCH] Refactor preview code
Date: Thu, 02 Oct 2025 19:02:54 +0300

[Message part 1 (text/plain, inline)]

Hello,

This is another follow-up bug-report/patch from:
https://lists.gnu.org/archive/html/auctex-devel/2025-08/msg00026.html

This patch (last one before preview-point implementation) refactors code for creating and starting preview processes. It also changes how the variable `preview-dvipng-command` is interpreted. If the value of this variable is a function symbol, it is called to get the command string. This is done for two reasons:
1. Cleaner code. Previously, some parts of the actual command were stitched on in `preview-start-dvipng`.
2. It allows using other processes and dynamics commands. For example (as documented in `preview-dvisvgm-command`) one can do

(setq preview-dvipng-command    #'preview-dvisvgm-command
      preview-dvipng-image-type 'svg)

to generate svg images instead of png ones.

I also attach a second patch (which can also be merged with the first) which renames all `dvipng` symbols in the refactored code to `dvi*` symbols. This is purely cosmetic (to avoid legacy-looking-code), so I wasn't sure if it should be done/included at all. However, it feels more natural since the actual refactored code is not specific to png (the process/image type are controlled via the previous two variables). I included obsolete aliases for old symbols for backward compatibility, but these can alternatively be made simple aliases if the patch is deemed appropriate. 

Best regards,
-- Al

[Refactor-code-and-generalize-dvi-conversion.patch (text/x-patch, attachment)]

[Rename-dvipng-symbols-to-dvi.patch (text/x-patch, attachment)]

Message #8 received at 79559 <at> debbugs.gnu.org (full text, mbox):

From: Al Haji-Ali <abdo.haji.ali <at> gmail.com>
To: 79559 <at> debbugs.gnu.org
Cc: Ikumi Keita <ikumi <at> ikumi.que.jp>, Arash Esbati <arash <at> gnu.org>
Subject: Re: [PATCH] Refactor preview code
Date: Sun, 12 Oct 2025 08:33:11 +0200

On 02/10/2025, Al Haji-Ali wrote:
> This patch (last one before preview-point implementation) refactors code for creating and starting preview processes.

@Arash, @Ikumi, please let me know if there's anything I can do to make reviewing this patch easier (like splitting or joining changes). 

Thanks,
-- Al

Message #11 received at 79559 <at> debbugs.gnu.org (full text, mbox):

From: Ikumi Keita <ikumi <at> ikumi.que.jp>
To: Al Haji-Ali <abdo.haji.ali <at> gmail.com>
Cc: Arash Esbati <arash <at> gnu.org>, 79559 <at> debbugs.gnu.org
Subject: Re: [PATCH] Refactor preview code
Date: Thu, 16 Oct 2025 22:10:51 +0900

Hi Al,

>>>>> Al Haji-Ali <abdo.haji.ali <at> gmail.com> writes:
> On 02/10/2025, Al Haji-Ali wrote:
>> This patch (last one before preview-point implementation) refactors
>> code for creating and starting preview processes.

> @Arash, @Ikumi, please let me know if there's anything I can do to
> make reviewing this patch easier (like splitting or joining changes).

Sorry for further delay, I've been laid up with a terrible cold since
Sunday night, and I've finally managed to get back to reviewing.

Your proposal basically looks good to me and I think we can accept it.
There is one minor concern: why do you treat dvisvgm as if it were a
variant of dvipng? It seems more intuitive to introduce
`preview-dvisvgm-process-setup', `preview-start-dvisvgm' etc.

-(defun preview-dvipng-process-setup ()
-  "Set up dvipng process for conversion."
+(defun preview-dvi*-process-setup-1 (img-type start sentinel)
+  "Setup process of a DVI converter to IMG-TYPE.
+START is the function that actually starts the process and SENTINEL is
+the used `TeX-sentinel-function'."

The last sentence seems to lack a word between "the" and "used" (and
possibly "by" is also missing?)

Regards,
Ikumi Keita
#StandWithUkraine #StopWarInUkraine
#Gaza #StopGenocide #CeasefireNOW

Message #14 received at 79559 <at> debbugs.gnu.org (full text, mbox):

From: Al Haji-Ali <abdo.haji.ali <at> gmail.com>
To: Ikumi Keita <ikumi <at> ikumi.que.jp>
Cc: Arash Esbati <arash <at> gnu.org>, 79559 <at> debbugs.gnu.org
Subject: Re: [PATCH] Refactor preview code
Date: Thu, 16 Oct 2025 15:42:57 +0200

Hello Ikumi,

On 16/10/2025, Ikumi Keita wrote:
> Sorry for further delay, I've been laid up with a terrible cold since
> Sunday night, and I've finally managed to get back to reviewing.
No worries, I hope you are feeling better!

> Your proposal basically looks good to me and I think we can accept it.
> There is one minor concern: why do you treat dvisvgm as if it were a
> variant of dvipng? It seems more intuitive to introduce
> `preview-dvisvgm-process-setup', `preview-start-dvisvgm' etc.
The point is that the dvipng functions are exactly the same as the
dvisvgm functions, at least in the refactored code where everything is
controlled with `preview-dvipng-image-type` and
`preview-dvipng-command`.

I agree that dvisvgm being a variant of dvipng is weird, that is why I
proposed in the second patch I attached to rename all symbols to
`preview-dvi*-process-setup`, `preview-start-dvi*` etc so that they
include both dvipng and dvisvgm as two customization of a common
interface (the obsolete aliases would ensure code is still backward
compatible).

The other option, as you said, is to introduce new functions which call
helper functions with TYPE and COMMAND arguments fixed to those
compatible with dvipng or dvisvgm.

This is what I did first (I can probably recover it from my
preview-point branch), but I found it to be too cumbersome as I had to
pass TYPE and COMMAND from function to functions to sentinels (and had
to change the interface of existing functions to accept those
arguments). It also required some code duplication.

Overall, it felt more natural to treat the dvi conversion as the key
feature, rather than the output file which can be customized.

Let me know if you disagree, and what you think about the second path
renaming functions.

> -(defun preview-dvipng-process-setup ()
> -  "Set up dvipng process for conversion."
> +(defun preview-dvi*-process-setup-1 (img-type start sentinel)
> +  "Setup process of a DVI converter to IMG-TYPE.
> +START is the function that actually starts the process and SENTINEL is
> +the used `TeX-sentinel-function'."
>
> The last sentence seems to lack a word between "the" and "used" (and
> possibly "by" is also missing?)

It's my awkward phrasing.

Perhaps simply: "START is the function that actually starts the process
with `TeX-sentinel-function' set to SENTINEL."

-- Al

Message #17 received at 79559 <at> debbugs.gnu.org (full text, mbox):

From: Ikumi Keita <ikumi <at> ikumi.que.jp>
To: Al Haji-Ali <abdo.haji.ali <at> gmail.com>
Cc: Arash Esbati <arash <at> gnu.org>, 79559 <at> debbugs.gnu.org
Subject: Re: [PATCH] Refactor preview code
Date: Mon, 20 Oct 2025 18:43:58 +0900

Hi Al,

>>>>> Al Haji-Ali <abdo.haji.ali <at> gmail.com> writes:
> I agree that dvisvgm being a variant of dvipng is weird, that is why I
> proposed in the second patch I attached to rename all symbols to
> `preview-dvi*-process-setup`, `preview-start-dvi*` etc so that they
> include both dvipng and dvisvgm as two customization of a common
> interface (the obsolete aliases would ensure code is still backward
> compatible).

> The other option, as you said, is to introduce new functions which call
> helper functions with TYPE and COMMAND arguments fixed to those
> compatible with dvipng or dvisvgm.

> This is what I did first (I can probably recover it from my
> preview-point branch), but I found it to be too cumbersome as I had to
> pass TYPE and COMMAND from function to functions to sentinels (and had
> to change the interface of existing functions to accept those
> arguments). It also required some code duplication.

> Overall, it felt more natural to treat the dvi conversion as the key
> feature, rather than the output file which can be customized.

> Let me know if you disagree, and what you think about the second path
> renaming functions.

OK, your second patch approach seems reasonable to me. I'm still busy
catching up on various things that piled up while I was sick, so I
haven't been able to get to it yet, but I'll take care of it eventually.

>> -(defun preview-dvipng-process-setup ()
>> -  "Set up dvipng process for conversion."
>> +(defun preview-dvi*-process-setup-1 (img-type start sentinel)
>> +  "Setup process of a DVI converter to IMG-TYPE.
>> +START is the function that actually starts the process and SENTINEL is
>> +the used `TeX-sentinel-function'."
>> 
>> The last sentence seems to lack a word between "the" and "used" (and
>> possibly "by" is also missing?)

> It's my awkward phrasing.

> Perhaps simply: "START is the function that actually starts the process
> with `TeX-sentinel-function' set to SENTINEL."

Looks good.

Regards,
Ikumi Keita
#StandWithUkraine #StopWarInUkraine
#Gaza #StopGenocide #CeasefireNOW

Message #20 received at 79559 <at> debbugs.gnu.org (full text, mbox):

From: Ikumi Keita <ikumi <at> ikumi.que.jp>
Cc: 79559 <at> debbugs.gnu.org, Arash Esbati <arash <at> gnu.org>,
 Al Haji-Ali <abdo.haji.ali <at> gmail.com>
Subject: Re: bug#79559: [PATCH] Refactor preview code
Date: Sun, 26 Oct 2025 23:51:03 +0900

Hi all,

>>>>> Ikumi Keita <ikumi <at> ikumi.que.jp> writes:
> OK, your second patch approach seems reasonable to me. I'm still busy
> catching up on various things that piled up while I was sick, so I
> haven't been able to get to it yet, but I'll take care of it eventually.

Now I have installed your proposal into the git repo, with slight
modifications to suppress byte compiler warnings. Thanks so much for
your cooperation!
I'll close this bug.

Regards,
Ikumi Keita
#StandWithUkraine #StopWarInUkraine
#Gaza #StopGenocide #CeasefireNOW

Message #23 received at 79559 <at> debbugs.gnu.org (full text, mbox):

From: Arash Esbati <arash <at> gnu.org>
To: Ikumi Keita <ikumi <at> ikumi.que.jp>
Cc: Al Haji-Ali <abdo.haji.ali <at> gmail.com>, 79559 <at> debbugs.gnu.org
Subject: Re: bug#79559: [PATCH] Refactor preview code
Date: Sun, 26 Oct 2025 17:49:49 +0100

Hi all,

Ikumi Keita <ikumi <at> ikumi.que.jp> writes:

>>>>>> Ikumi Keita <ikumi <at> ikumi.que.jp> writes:
>> OK, your second patch approach seems reasonable to me. I'm still busy
>> catching up on various things that piled up while I was sick, so I
>> haven't been able to get to it yet, but I'll take care of it eventually.
>
> Now I have installed your proposal into the git repo, with slight
> modifications to suppress byte compiler warnings. Thanks so much for
> your cooperation!
> I'll close this bug.

Thanks for working and installing this.  Should we mention the user
level changes of this patch in NEWS.org?  I'm thinking about the changes
in the custom variables.

I suggest we also add

  :package-version '(auctex . "14.1.1")

to the custom options touched.

Best, Arash

Message #26 received at 79559 <at> debbugs.gnu.org (full text, mbox):

From: Al Haji-Ali <abdo.haji.ali <at> gmail.com>
To: Arash Esbati <arash <at> gnu.org>, Ikumi Keita <ikumi <at> ikumi.que.jp>
Cc: 79559 <at> debbugs.gnu.org
Subject: Re: bug#79559: [PATCH] Refactor preview code
Date: Mon, 27 Oct 2025 14:23:49 +0100

Hello Arash,

On 26/10/2025, Arash Esbati wrote:
> Thanks for working and installing this.  Should we mention the user
> level changes of this patch in NEWS.org?  I'm thinking about the changes
> in the custom variables.

I'll add those and send a patch. I also noticed that the documentation
in preview-latex.texi needs updating.

> I suggest we also add
>
>   :package-version '(auctex . "14.1.1")

Just to note, I thought the changes would be included in 14.2, so I use
that for `define-obsolete-*-alias` calls. Should I change that to 14.1.1?

Best regards,
-- Al

Message #29 received at 79559 <at> debbugs.gnu.org (full text, mbox):

From: Arash Esbati <arash <at> gnu.org>
To: Al Haji-Ali <abdo.haji.ali <at> gmail.com>
Cc: Ikumi Keita <ikumi <at> ikumi.que.jp>, 79559 <at> debbugs.gnu.org
Subject: Re: bug#79559: [PATCH] Refactor preview code
Date: Mon, 27 Oct 2025 23:51:56 +0100

Hi Al,

Al Haji-Ali <abdo.haji.ali <at> gmail.com> writes:

> I'll add those and send a patch. I also noticed that the documentation
> in preview-latex.texi needs updating.

Great, thanks.

> Just to note, I thought the changes would be included in 14.2, so I use
> that for `define-obsolete-*-alias` calls. Should I change that to 14.1.1?

We don't have a formal agreement on the versioning.  In the past, we
usually stepped the last number in MAJOR.MINOR.PATCH couple of time and
then the minor.

But since we're adding more functionality, I think you're right and we
should go for 14.2 when we land your next patch.

Best, Arash

Message #32 received at 79559 <at> debbugs.gnu.org (full text, mbox):

From: Al Haji-Ali <abdo.haji.ali <at> gmail.com>
To: Arash Esbati <arash <at> gnu.org>
Cc: Ikumi Keita <ikumi <at> ikumi.que.jp>, 79559 <at> debbugs.gnu.org
Subject: Re: bug#79559: [PATCH] Refactor preview code
Date: Tue, 28 Oct 2025 07:34:39 +0100

[Message part 1 (text/plain, inline)]

Hello both,

On 27/10/2025, Arash Esbati wrote:
> But since we're adding more functionality, I think you're right and we
> should go for 14.2 when we land your next patch.

Attached is the patch announcing the changes and updating the
documentation where `dvipng` was previously mentioned.

Best regards,
-- Al

[Update-documentation-to-reflect-dvi-rename.patch (text/x-patch, attachment)]

Message #35 received at 79559 <at> debbugs.gnu.org (full text, mbox):

From: Arash Esbati <arash <at> gnu.org>
To: Al Haji-Ali <abdo.haji.ali <at> gmail.com>
Cc: Ikumi Keita <ikumi <at> ikumi.que.jp>, 79559 <at> debbugs.gnu.org
Subject: Re: bug#79559: [PATCH] Refactor preview code
Date: Tue, 28 Oct 2025 10:21:04 +0100

[Message part 1 (text/plain, inline)]

Hi Al,

Al Haji-Ali <abdo.haji.ali <at> gmail.com> writes:

> Attached is the patch announcing the changes and updating the
> documentation where `dvipng` was previously mentioned.

thank you.  I slightly touched your patch (see file attached) and pushed
it.  Please let me know if you're happy with it, and we can close this
and move to the next round :-)

Best, Arash

[Additions.diff (text/x-patch, attachment)]

Message #38 received at 79559 <at> debbugs.gnu.org (full text, mbox):

From: Al Haji-Ali <abdo.haji.ali <at> gmail.com>
To: Arash Esbati <arash <at> gnu.org>
Cc: Ikumi Keita <ikumi <at> ikumi.que.jp>, 79559 <at> debbugs.gnu.org
Subject: Re: bug#79559: [PATCH] Refactor preview code
Date: Tue, 28 Oct 2025 22:06:46 +0100

On 28/10/2025, Arash Esbati wrote:
> thank you.  I slightly touched your patch (see file attached) and pushed
> it.  Please let me know if you're happy with it, and we can close this
> and move to the next round :-)

Thanks Arash for the correction/additions. They all look good to me.

Best regards,
-- Al

Message #43 received at 79559-done <at> debbugs.gnu.org (full text, mbox):

From: Arash Esbati <arash <at> gnu.org>
To: Al Haji-Ali <abdo.haji.ali <at> gmail.com>
Cc: Ikumi Keita <ikumi <at> ikumi.que.jp>, 79559-done <at> debbugs.gnu.org
Subject: Re: bug#79559: [PATCH] Refactor preview code
Date: Wed, 29 Oct 2025 12:40:47 +0100

Al Haji-Ali <abdo.haji.ali <at> gmail.com> writes:

> Thanks Arash for the correction/additions. They all look good to me.

Great, thank you for making this all happen.  I'm closing this report.

Best, Arash

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.