Alberth wrote:

Basic idea seems fine, as is the introduction of the OnMouseDrag() callback.

The actual highlighting in the windows however is not very convincing. In the order gui and the group gui, the additional > looks a lot like a normal gui element, in the screen shot you cannot see it 'jumping out', attracting the attention of the user. The depot gui has the same problem, the arrows are mostly invisible.

If you can improve the highlighting that would be great.

Furthermore, please make sure you follow coding style. Several lines have white space at the end of the line. Also, all functions should have doxygen docs, 'case' statements are seperated from each other by an empty lines, and one statement at a line.
Last but not least, the > in the order gui is glitchy, when inserting before the top-row, the > is drawn outside the matrix widget border.

This comment was imported from FlySpray: https://bugs.openttd.org/task/3705#comment8032

sbr wrote:

Thanks for reviewing this patch; I'll take more care of the coding style.

I've a new proposal for the group highlighting: filling the cell background and writing the text in white when dragged over.
That's consistent with toolbar and dropdown widgets if filled in black. However for the group gui, I find black a little bit aggressive.

Attached are three version of the group gui, highlighted with black, _colour_gradient[COLOUR_GREY][3] and _colour_gradient[COLOUR_GREY][7].

Attachments

• group-gui-highlighting.png (25.95 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3705#comment8073

Alberth wrote:

I forgot to mention this the previous time:
When looking into code style, also check the multi-line comments. In the OpenTTD style, every line starts with a * . Thank you!

With respect to your new proposal:
I agree that giving the background a different colour is a nice solution. Highlighting the text I am less sure of. If you drag&drop in the same list (eg re-order the active newgrfs in the newgrf gui), another entry in the same list is also highlighted, which may cause confusion.

The black highlighted background seems quite ugly, so I'd use one of the other two.
If you also highlight the text, the darker background seems better since it gives more contrast. If you leave the text black, the lighter background may be better (again since it gives more contrast). You probably need to program it, and experiment a little to find the best solution.

Highlighting may give problems in the new NewGRF gui with its black background, but I am thinking to change that anyway. :)
Looking forward to your patches.

This comment was imported from FlySpray: https://bugs.openttd.org/task/3705#comment8074

sbr wrote:

I choose to test highlighting with a light gray rectangle without changing the text colour. If the object is dragged over itself (or over its group) nothing is drawn.

In the group gui, the cell is highlighted.
In the order gui, a line is drawn between the orders were the dragged one will be inserted.
In the depot gui, an highlighted space is inserted in the train where the wagon will be inserted. It's size depends of the width of the dragged wagon.

I'm not yet convinced by the order gui highlighting; I'll try to find something better.

Concerning the new NewGRF gui, maybe it can be highlighted with the blue currently used to mark the selected file.
The code used to move orders might be "borrowed" to allow the reordering of loaded NewGRF files. I've already thought of it but had not took time to do it.

About the codestyle, I think most of the errors are fixed (at least your python script doesn't complain anymore).

The code logic didn't change; the handler patch and its usages in the other patches are nearly the same since the first version. I think it's now for me more a matter of finding the right look for the highlight.

Please find a new screenshot and updated patches against r19880.

Attachments

• highlight-draganddrop-destination_r19880.png (59.49 KiB)
• highlight-draganddrop-destination_hg-e25a96e39d2d_00add-drag-event-handler.patch (1.58 KiB)
• highlight-draganddrop-destination_hg-e25a96e39d2d_01dragged-order.patch (2.42 KiB)
• highlight-draganddrop-destination_hg-e25a96e39d2d_01dragged-vehicle-in-group.patch (5.31 KiB)
• highlight-draganddrop-destination_hg-e25a96e39d2d_01dragged-vehicle-in-depot.patch (7.24 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3705#comment8082

sbr wrote:

I proposed a patch to enable NewGRF active files drag-and-drop in #3854.
I also made a patch that highlight the destination of the dragged NewGRF files.

Please find attached a patch against r19884.

Attachments

• highlight-draganddrop-destination_hg-e706da6b8ddc_01dragged-newgrf-file.patch (2.88 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3705#comment8083

Alberth wrote:

I just pushed the general infra structure, and the depot and order gui highlighting to trunk (I made some changes to the latter).
The group gui contains some errors in my view. In a gui without any groups (that is, only the 'ungrouped' and 'all' groups), I get a highlight when dragging a train from the right one of both groups, yet nothing actually changes.
Note that doing this with a train that is in some group, does cause a change, so in that situation, the highlighting is correct.

Patches were good, except lines getting split always get an indent of two tabs at the second line, and you use a non-ascii character for ".." which does not seem necessary.

Thanks for your work.

This comment was imported from FlySpray: https://bugs.openttd.org/task/3705#comment8085

Krille wrote:

I like the already committed changes, however there are a few issues.

Concerning the orders window:
When aborting dragging of an order with the Esc key then the highlighting isn't cleared. This also happens for the (rather constructed) case of pressing a key which opens a toolbar while dragging.

Not caused by these changes, but somewhat annoying: You can't drag the selected order. For trains it will only change the stop location and do nothing for the other vehicle types. It should change the stop location only on mouse up. If there's no mouse up but the mouse is moved then it should start dragging. Is that possible? Buttons generally already react on mouse down, so there's no way to abort a misdirected click. That could also be improved.

Concerning the depot window:
There's no highlighting when dragging a wagon to an empty row. This may create the impression that the wagon couldn't be dragged there.

For dual headed engines the position after the rear head is highlighted but on drop the wagon actually gets placed before the head.

This comment was imported from FlySpray: https://bugs.openttd.org/task/3705#comment8097

michi_cc closed the ticket.

Reason for closing: Implemented

Final parts in r24125.

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

Krille wrote:

Well, there are some points left from my previous comment and some new points:

Concerning the group window:
Dragging a vehicle to an empty space in the group list creates a new group, but there's no highlighting.

Dragging a grouped vehicle to the "All vehicles" group will remove it from the group, but there's no highlighting.

Dragging a grouped vehicle to the group it's already in is a no-op, but there is highlighting.

Concerning the orders window:
Not caused by these changes, but somewhat annoying: You can't drag the selected order. For trains it will only change the stop location and do nothing for the other vehicle types. It should change the stop location only on mouse up. If there's no mouse up but the mouse is moved then it should start dragging. Is that possible? Buttons generally already react on mouse down, so there's no way to abort a misdirected click. That could also be improved.

Concerning the depot window:
There's no highlighting when dragging a wagon to an empty row. This may create the impression that the wagon couldn't be dragged there.

For dual headed engines the position after the rear head is highlighted but on drop the wagon actually gets placed before the head.

This comment was imported from FlySpray: https://bugs.openttd.org/task/3705#comment11100