unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* 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).