Alberth wrote:

Nice patch.

With respect to the code of the patch:
You can merge the two conditions at the top of OnDragDrop(), and save an tab indent for almost the whole function.
Also the check in "} else if (from_offset > to_offset) {" is not needed, as you already established that both offsets are not the same.

As for the functionality, the re-ordering of the active grfs works great.
However, a user may also try to move grfs from the non-active list to the active list, or vice versa.
currently that does not work. Would it be possible to implement that feature?

This comment was imported from FlySpray: https://bugs.openttd.org/task/3854#comment8086

sbr wrote:

I've packed the if statement in the first patch and renamed variables to better stick with codestyle guidelines.

The second patch insert an available file into active list. The file is inserted just before the one that is hovered. To permit insertion at the end of the list, an empty line terminate the list.

The third patch remove an active file by dragging it over the available list. It only call OnClick(, SNGRFS_REMOVE, ).

Please find updated patches against r19892.

Attachments

• drag-newgrf-files_hg-a3cfaeee08f4_00drag-active-newgrf-files.patch (2.74 KiB)
• drag-newgrf-files_hg-a3cfaeee08f4_01drag-available-to-active.patch (6.00 KiB)
• drag-newgrf-files_hg-a3cfaeee08f4_02drag-active-to-available.patch (0.48 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3854#comment8089

Alberth wrote:

Code seems to work nicely, although you really miss the feedback of highlighting the target when dropping. But more on that at the end.

The patch itself has two problems, a real one (code-wise) and one small one (patch-wise).

The small one is the shifting of code in the 2nd patch, that you just added in the 1st patch.
It really makes the patch become a lot bigger. It took me 15-20 minutes to figure out nothing significant had changed.

If you had kept the nested if statements in the first patch, the shifting of code would not have been needed, and the second patch would have become much smaller and easier to understand. I know I said that you can combine them, and indeed you can, but that was before you added more patches (which is good btw, separate patches for separate functionality is much appreciated).
In general, try to move as little code around as possible, even if the code is temporarily a bit non-optimal (just mention it in the ticket comments, so we know why you decided against it).
If you cannot prevent it, make a separate patch that only moves code without changing it. The idea is that the patches must be as easy as possible. Code movement in itself is not a problem, humans are just very bad at detecting that nothing changed, other than white space. By making it a separate patch (and mentioning that you only move code) I can simply apply it without further study. Even big blocks are then not a problem.

The extra line at the bottom of the actives that you introduce was also a puzzle to me. After some thinking, I concluded that your approach is the right one. It is consistent with the order gui, and a whole row is bigger than a 1/2 row at the top and the bottom.

The real show stopper was the duplication of the add-code.
The reason that there is a lot of code in the 'case' statements in eg OnClick(), is because that code is needed at only one place, and it serves little purpose to wrap it all in a method, and then call that method from one place in the program.
With your second patch however, you are adding a second way of moving an item from the availables to the actives list. The new way is more generic, as you may need to put it somewhere in the middle, instead of only at the end.
With two points where you need to have the same functionality, you don't make a copy of the code. Instead you move the code to a new function, generalize it so you can insert the moved item at any point in the actives list, and then call that function from two places.

In a sense, the third patch also does that. It does not duplicate the same function, instead it simply calls code already available.

PS Although this issue is only about added drag/drop, without adding highlighting, I find drag/drop quite non-useful without seeing what I am doing.
Therefore, I'd like to propose to extend this issue to include the highlighting patch as well.
Your patches here will grow by one patch, but it is not needed in the other issue (# 3705) any more.
What do you think?
(if the patch for highlighting is still the same, I can pull it from the other issue after applying all patches here)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3854#comment8101

michi_cc closed the ticket.

Reason for closing: Implemented

In r24126.

This comment was imported from FlySpray: https://bugs.openttd.org/task/3854