Comments

arnested’s picture

Version: 7.x-1.x-dev » 7.x-2.x-dev
Component: Multiple values - allow new values » User interface

Changed this feature request from version 1.x to 2.x.

Being able to drag and drop the selected terms to a new order would still be awesome!

drupalok’s picture

Issue summary: View changes

one more that would like this feature.... oops... it was me who started the thread... lol :)

delphian’s picture

StatusFileSize
new496 bytes

Adding a patch that defaults the sort order to the taxonomy weight.

dxx’s picture

Good feature! :)

jazzdrive3’s picture

Status: Active » Needs review
StatusFileSize
new3.42 KB

Here is a patch that uses the jQuery UI sortable library. It's just a change of a few lines of code and seems to work well.

alison’s picture

+1!!

alison’s picture

Patch won't apply for me via drush patch-add -- the patch file itself seems to have a little different format than what I usually see, but I'm not an expert, could just be me (mainly all the descriptive text at the top -- I don't think that's usually there?).

Patch format needs some TLC -- it has the file paths, etc. from the author's own system, and descriptive text at the top that shouldn't be there (although I rather like having it so handy!).

LNakamura’s picture

@alisonjo2786 - you can apply #5 using the following command (in the autocomplete_deluxe module directory):
patch -p7 < /path/to/autocomplete_deluxe-ordering-1480126-5.patch

@jazzdrive3 - what is the purpose of this function declaration in autocomplete_deluxe.js in #5?

function _autocomplete_deluxe_sort_update(event, ui) {

}

Other than the above, your patch looks (and functions) great!

LNakamura’s picture

Status: Needs review » Needs work
alison’s picture

Yes, thank you, that applied perfectly -- but I don't *think* it is proper coding standards to have the top of the patch file be the way it is? Even just based on the fact that I can't apply it via drush patch-add (drush patches.make tool).

[EDIT] And now that I re-read, the site name is in the path in the patch file ("blastr") -- that should be cleared out :)
[EDIT 2] And file perms are being set by the patch (100644).
>> I'll comment once more in a minute with a new patch suggestion.

But, I love love love that this functionality has been built, I'm seriously ecstatic :D Thank you!

alison’s picture

StatusFileSize
new2.22 KB

EDIT: Revised version coming in a moment.......

alison’s picture

Take 2 -- got the author attrib right this time (I think, lol).

(All I was trying to do is clean up the paths and a couple other tidying up things.)

alison’s picture

Status: Needs work » Needs review
alison’s picture

Removed aforementioned empty function.

LNakamura’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new273 bytes

Thanks, @alisonjo2786!

I tested #14 (#13), and it works great.

I think you meant to call your last interdiff "interdiff-1480126-5-13.txt", so I'm including a version with that filename and re-organizing the files a bit.

alison’s picture

@LNakamura Yes I really did lol, and I noticed it after posting (I miscounted b/c I forgot my Issue Status Change comment), but then I saw how many notifications my comments had already generated for this thread, and didn't re-re-post haha. Thank you / you're welcome!

EDIT: Jk, i meant for it to be called *-12-14, b/c it's the interdiff of 12-14 -- the patch in #14 is an update to the patch in #12, all it does is remove the empty function (#12 is already an update to #5). Sorry!!

alison’s picture

Status: Reviewed & tested by the community » Needs review
StatusFileSize
new2.55 KB
new4.1 KB

Okay, one more try... this time it's straight-up interdiff with #5, instead of incremental 5-12 and 12-13 (14)... I hope this is correct! Sorry!! (FWIW I think I learned a lot in the last 24 hours haha)

LNakamura’s picture

@alisonjo2786 - you're welcome :-)

So it looks like #17 is the same as #13, but you just wanted to get the numbering correct? Thanks for that.

I'm really puzzled about this interdiff, which is similar to your first one. How are you generating them? You're correct that you want to compare with #5, but the output should look like your 2nd interdiff (12 vs. 13), and the one I provided for 5 vs. 13. This is the process I follow: https://www.drupal.org/documentation/git/interdiff

Glad you're learning a lot! This is my first time co-maintaining a module (as of last Friday), so I'm learning a lot as well :-)

alison’s picture

Yay for learning! :D

So, I've been following that documentation, and a few others in the "patching" part of d.o documentation, very closely -- I'm still missing bits sometimes, but I'm attempting to follow it to the letter. I used the interdiff command, b/c my Linux OS already had patchutils installed. I could be (and probably am!) wrong, buuuuut I think your 5-13 interdiff actually wasn't an accurate representation of the differences between #5 and #13 -- *-5-13 only showed the removal of the empty function, which is the same as #12-13, but the difference between #5 and #13 (or #17) has other stuff (fixing the paths to the module).

[thinking and reading/looking and typing at the same time] But something's not quite right. I'm going to look *once more*, I may have one more version, if that's not too horribly obnoxious.

(Oh and, #17 was meant to be the same as #13 + #12)

LNakamura’s picture

@alisonjo2786,

Hmm.... you might be right, but my understanding is that the interdiff should show the differences between the versions of the patch from the perspective of what the patch actually does. File paths, etc., are not relevant to what the patch does to change the source code.

I'll see if I can get an authority to weigh in here.

alison’s picture

So I started fresh this time -- my interdiff is still confusing to *my* eyes, but I followed all the steps -- I think it's just little things that are different (whitespace and/or starts-and-ends of files) that are causing it to be lengthy... I think!

Attached is a fresh patch and interdiff, *-5-21. I'll cross my fingers and wait :)

alison’s picture

OMG I created new file names but forgot to re-upload, I'm so sorry, dammit dammit dammit...

This comment and file attachments are identical to the previous, but with correct comment number.

----------------------------
So I started fresh this time -- my interdiff is still confusing to *my* eyes, but I followed all the steps -- I think it's just little things that are different (whitespace and/or starts-and-ends of files) that are causing it to be lengthy... I think!

Attached is a fresh patch and interdiff, *-5-22. I'll cross my fingers and wait :)

alison’s picture

@LNakamura re: what the interdiff should include -- OH well that makes total sense, I didn't realize that, I was following "then run this command" steps so closely I must have missed some context info in the documentation, sorry about that.

Here's what I ran, in order, just to put it all out there :) (copied from my own piece of documentation that I created yesterday/today during this process, that's why there's extra "talking to myself" info)

git clone --branch 7.x-2.x https://git.drupal.org/project/autocomplete_deluxe.git
cd autocomplete_deluxe/
git co -b 1480126-allow-drag-and-drop-reordering

// Code changes... then...

// NOTE: In this case, I was just creating a patchfile for a fix that someone
// else wrote.  If I were the fixer, I'd omit the author info, b/c I'd be marked
// as the author automatically.

git commit --author="Matt Robison <jazzdrive3@348785.no-reply.drupal.org>" -am 'Issue #1480126 by jazzdrive3: Add re-ordering values in autocomplete_deluxe field widget.'

// Refresh the issue thread to see what your comment number will be (the -XX at
// end of patch file name = comment # you'll have when you upload the patch).
git format-patch origin/7.x-2.x --stdout > autocomplete_deluxe-ordering-1480126-13.patch

// If your patch is an update to an existing patch, create an interdiff.
// NOTE: You may need to download the prior patch to this directory for patchutils to be able to "find" and compare.
interdiff autocomplete_deluxe-ordering-1480126-5.patch autocomplete_deluxe-ordering-1480126-17.patch > interdiff-1480126-5-17.txt
damienmckenna’s picture

Just to weigh in here - patches #14 and #17 (and 21 and 22) are identical. Also, the interdiff should only contain the differences between the two patches, so the fact that it is reverting the changes to the CSS file even though both of the actual patches contain the same changes indicates it was made incorrectly.

A tip I use for making patches - get a fresh git checkout of the branch in question (git checkout .; git checkout 7.x-2.x; git pull), download & apply the first patch (curl [patch URL] | patch -p1), confirm it applied correctly if not then reroll the patch, presuming it applied correctly stage the changes (git add *), make the additional changes, use "git diff > ~/Downloads/autocomplete_deluxe-n1480126-21.interdiff.txt" to create the interdiff and "git diff HEAD > ~/Downloads/autocomplete_deluxe-n1480126-21.patch" to create the patch.

alison’s picture

Okay, that makes sense.

I wonder, could it be that it thinks it's doing "new changes" because it thinks the changes are to completely different module files, b/c of the file path differences?

(I'm not saying the interdiff should be as lengthy as mine is, I'm trying to figure out why the normal interdiff command isn't giving me the intended results -- see also, my sequence of commands in comment #23.)

Thank you!

[UPDATE] Yeah I messed up numbering and tried to re-do the uploading/commenting, sorry if that was more problematic than helpful!

LNakamura’s picture

Thanks, @DamienMcKenna!

@alisonjo2786 the interdiff command has at least one option (-p N - do: interdiff --help) which would allow you to ignore things that using git diff (per https://www.drupal.org/documentation/git/interdiff and Damien, above) ignores without requiring options.

LNakamura’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new273 bytes

@alisonjo2786 - I'm uploading a new (simple) interdiff-1480126-5-22.txt and cleaning up the displayed files - I think we're good on patches and interdiffs now :-) Thanks for your contributions to this issue, and I'm glad it's been a good learning experience for you! (For me, as well!)

LNakamura’s picture

Status: Reviewed & tested by the community » Fixed

Merged - thanks, all!

alison’s picture

Cool, thanks so much for taking the time to explain / break it down for me :) I'm also thrilled to have this functionality now! (THANK YOU @jazzdrive3 !!)

LNakamura’s picture

You're welcome! :-)

jrabeemer’s picture

Congrats. @jazzdrive3 wrote this for Syfy and we use it without issue. :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

m.abdulqader’s picture

Version: 7.x-2.x-dev » 8.x-1.x-dev

Can we have this for Drupal 8

damienmckenna’s picture

Version: 8.x-1.x-dev » 7.x-2.x-dev

Please create a new issue for that and cross-reference this one.

alison’s picture

@m.abdulqader FWIW, the drag-and-drop reordering works for me on 8.x -- it looks like they make commits on both versions at the same time last year:
https://www.drupal.org/commitlog/commit/16390/95cd25bf4d574dd0a581e93c58...

(Also noting it here in case anyone pops into this issue -- I came back to this issue today, it hadn't even occurred to me to try it on my D8 sites -- I hadn't thought about the issue in so long -- and I was like, oh hey, it already works..... :) )