* bug#24676: 25.1; `completion-pcm-all-completions' should not reverse order of candidates
@ 2016-10-12 16:56 Drew Adams
2016-10-17 8:19 ` Michael Heerdegen
2016-10-17 16:29 ` Stefan Monnier
0 siblings, 2 replies; 6+ messages in thread
From: Drew Adams @ 2016-10-12 16:56 UTC (permalink / raw)
To: 24676
Because of this part, at the end of the function,
`completion-pcm-all-completions' reverses the order of the candidates.
(let ((poss ()))
(dolist (c compl)
(when (string-match-p regex c) (push c poss))) ; <==== reverses
poss)
It should not do this. Users should be able to depend on whatever order
the candidates are already in (as returned by `all-completions').
For example, for buffer candidates, the default order from
`all-completions' is last-display-time order.
The fix is trivial:
(let ((poss ()))
(dolist (c compl)
(when (string-match-p regex c) (push c poss)))
(nreverse poss))
In GNU Emacs 25.1.1 (x86_64-w64-mingw32)
of 2016-09-17
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
`configure --without-dbus --without-compress-install CFLAGS=-static'
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#24676: 25.1; `completion-pcm-all-completions' should not reverse order of candidates
2016-10-12 16:56 bug#24676: 25.1; `completion-pcm-all-completions' should not reverse order of candidates Drew Adams
@ 2016-10-17 8:19 ` Michael Heerdegen
2016-10-17 16:29 ` Stefan Monnier
1 sibling, 0 replies; 6+ messages in thread
From: Michael Heerdegen @ 2016-10-17 8:19 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 24676
Hello,
> Because of this part, at the end of the function,
> `completion-pcm-all-completions' reverses the order of the candidates.
>
> (let ((poss ()))
> (dolist (c compl)
> (when (string-match-p regex c) (push c poss))) ; <==== reverses
> poss)
>
> It should not do this. Users should be able to depend on whatever order
> the candidates are already in (as returned by `all-completions').
>
> For example, for buffer candidates, the default order from
> `all-completions' is last-display-time order.
>
> The fix is trivial:
>
> (let ((poss ()))
> (dolist (c compl)
> (when (string-match-p regex c) (push c poss)))
> (nreverse poss))
Ah, this is the reason for the candidate list reversion I see. Yes,
this should IMO not happen.
Stefan, if you get the time, can you please have a look at whether the
suggested fix looks ok to you, and commit it if applicable?
Thanks,
Michael.
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#24676: 25.1; `completion-pcm-all-completions' should not reverse order of candidates
2016-10-12 16:56 bug#24676: 25.1; `completion-pcm-all-completions' should not reverse order of candidates Drew Adams
2016-10-17 8:19 ` Michael Heerdegen
@ 2016-10-17 16:29 ` Stefan Monnier
2016-10-18 8:11 ` Michael Heerdegen
1 sibling, 1 reply; 6+ messages in thread
From: Stefan Monnier @ 2016-10-17 16:29 UTC (permalink / raw)
To: Drew Adams; +Cc: 24676
> Because of this part, at the end of the function,
> `completion-pcm-all-completions' reverses the order of the candidates.
completion-pcm-all-completions doesn't have any such code.
After searching a bit I decided you meant completion-pcm--all-completions.
> (let ((poss ()))
> (dolist (c compl)
> (when (string-match-p regex c) (push c poss)))
> (nreverse poss))
Looks good, please install,
Stefan
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#24676: 25.1; `completion-pcm-all-completions' should not reverse order of candidates
2016-10-17 16:29 ` Stefan Monnier
@ 2016-10-18 8:11 ` Michael Heerdegen
2016-10-18 13:56 ` Drew Adams
0 siblings, 1 reply; 6+ messages in thread
From: Michael Heerdegen @ 2016-10-18 8:11 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 24676
Stefan Monnier <monnier@iro.umontreal.ca> writes:
> completion-pcm-all-completions doesn't have any such code. After
> searching a bit I decided you meant completion-pcm--all-completions.
Oh yes, I found that too, but forgot to mention.
>
> > (let ((poss ()))
> > (dolist (c compl)
> > (when (string-match-p regex c) (push c poss)))
> > (nreverse poss))
>
> Looks good, please install,
This doesn't seem to meet any of the criteria in
http://lists.gnu.org/archive/html/emacs-devel/2016-10/msg00007.html
so I think I'll have to commit to master.
Michael.
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#24676: 25.1; `completion-pcm-all-completions' should not reverse order of candidates
2016-10-18 8:11 ` Michael Heerdegen
@ 2016-10-18 13:56 ` Drew Adams
2016-10-18 14:22 ` Michael Heerdegen
0 siblings, 1 reply; 6+ messages in thread
From: Drew Adams @ 2016-10-18 13:56 UTC (permalink / raw)
To: Michael Heerdegen, Stefan Monnier; +Cc: 24676
> > > (let ((poss ()))
> > > (dolist (c compl)
> > > (when (string-match-p regex c) (push c poss)))
> > > (nreverse poss))
> >
> > Looks good, please install,
>
> This doesn't seem to meet any of the criteria in
> http://lists.gnu.org/archive/html/emacs-devel/2016-10/msg00007.html
> so I think I'll have to commit to master.
Thanks for fixing this, Michael.
Unfortunately, there is no way for my code that compensates for this bug to now know whether/when it has been fixed. No simple test such as `fboundp' or an Emacs version.
Of course, if you happened to decide to rename the function, there would be no problem...
And it's already a bit much that there is also a function `completion-pcm-all-completions' that does not even call `completion-pcm--all-completions', and that has a completely different signature. And the "internal" function has a nice doc string, but the "external" one has no doc string...
Perhaps you would consider taking this opportunity to find a better name for this "internal" function? Especially since it is "internal", probably few 3rd-party libraries would be affected by a name change.
In my case, I would not be referencing this "internal" function at all, except that I needed to work around this bug. If you did choose to rename it, I would not need to make any code changes, because I already protect this change with `(when (fboundp 'completion-pcm--all-completions)...)'.
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#24676: 25.1; `completion-pcm-all-completions' should not reverse order of candidates
2016-10-18 13:56 ` Drew Adams
@ 2016-10-18 14:22 ` Michael Heerdegen
0 siblings, 0 replies; 6+ messages in thread
From: Michael Heerdegen @ 2016-10-18 14:22 UTC (permalink / raw)
To: Drew Adams; +Cc: 24676, Stefan Monnier
Drew Adams <drew.adams@oracle.com> writes:
> Unfortunately, there is no way for my code that compensates for this
> bug to now know whether/when it has been fixed. No simple test such
> as `fboundp' or an Emacs version.
>
> Of course, if you happened to decide to rename the function, there
> would be no problem...
Hmm, that sounds reasonable. If nobody objects, I can do that, sure.
Michael.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-18 14:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-12 16:56 bug#24676: 25.1; `completion-pcm-all-completions' should not reverse order of candidates Drew Adams
2016-10-17 8:19 ` Michael Heerdegen
2016-10-17 16:29 ` Stefan Monnier
2016-10-18 8:11 ` Michael Heerdegen
2016-10-18 13:56 ` Drew Adams
2016-10-18 14:22 ` Michael Heerdegen
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).