Room editor's navigation bar: pulldown menu is inconvenient to scroll

[Crystal-Shard]

Describe the bug
This pulldown menu has inconveniently tiny "up" and "down" buttons, and does not support the mouse wheel.
The same applies to the pulldown for hotspots and for walkable areas and for regions.

AGS Version
3.6.0

Game
n/a

To Reproduce
Steps to reproduce the behavior:

1. Edit a room
2. Click on '....'
3. Select walk behinds
4. Click on '....'

Expected behavior
Show all 16 entries of this pulldown menu, if the window size permits.
If not, then allow the mouse wheel to scroll through this screen.
When closing and reopening the pulldown, start from the top (so that selecting the eraser is convenient).

Desktop (please complete the following information):

• OS: Windows
• Version 11

[ericoporto]

I have a code that "fixes" both the small buttons and the mouse wheel, but the way it works is totally overriding the behavior and paint from the toolstrip, and comes with quite a few small issues I couldn't fix, I think the best approach is to do the suggested TreeView in the room editor - I thought there was already an issue for it but I think I am mixing with the post from the forums...

[ivan-mogilko]

It's true that in the long run we better design better controls.

About existing one though, maybe there are ways to improve it a little without big changes to the code.

[ivan-mogilko]

Following are things that may be tried here, separately:

1. Make the pulldown menu bigger (higher), if display size permits, to accomodate at least 16 items (current limit for walkable areas, regions and walk-behinds); maybe more.
2. Support mousewheel scrolling.
3. Better scroll buttons, or add a scroll bar... If they are not easy to add to the existing control, may this be possible to not change the existing control at all, but to place this control onto some panel, and add our own buttones / scrollbar at the sides?

[ericoporto]

The mouse wheel fix I tried is this one : https://github.com/Orbmu2k/nvidiaProfileInspector/blob/master/nspector/Common/Helper/DropDownMenuScrollWheelHandler.cs

The button stuff I picked from here:
https://gist.github.com/ericoporto/9a6f65b49737b22f9e203df613499750

I wasn't very happy with the result so I ended up deleting the code - maybe it's still somewhere in my local git... But I think the best effort is the TreeView still.

[ivan-mogilko]

Well, I would definitely prefer to exhaust any available "simple" options, before going into custom paint (and maybe not go there at all).

Unfortunately, I never investigated how this pulldown works, so I'd need some time to make myself acquainted and understand the situation better.

If I understand correctly, this "pulldown" menu is created using standard ToolStripDropDownMenu class.
https://learn.microsoft.com/en-us/dotnet/api/system.windows.forms.toolstripdropdownmenu?view=windowsdesktop-8.0
The instances of this are created inside AddressBarExt::AddToolStripItemUpdate:

ags/Editor/AGS.Controls/AdressBarEx/AddressBarExt.cs

Lines 390 to 391 in ced9419

	//create the drop down menu
	tsDropDown = new ToolStripDropDownMenu();

Curiously, there seem to be mouse wheel handling event, so it does not work in our case for some reason?

ags/Editor/AGS.Controls/AdressBarEx/AddressBarExt.cs

Line 440 in ced9419

tsDropDown.MouseWheel += new MouseEventHandler(ScrollDropDownMenu);

Ah, there's even a comment: //This doesn't work :(

[Crystal-Shard]

Out of curiosity, why is this a ToolStrip instead of a regular drop down? To my knowledge, a regular drop down would automatically grow in size, and support the mousewheel, and even keyboard shortcuts (for the menus where NOT everything starts with the same letter).

[ivan-mogilko]

Out of curiosity, why is this a ToolStrip instead of a regular drop down?

What is a "regular drop down"?

The whole bar is a control copied from elsewhere.

[Crystal-Shard]

Sure, but I'm wondering why. What advantages does it offer over a regular Windows control, and does that weigh up against the fact that (unlike regular Windows controls) it doesn't appear to support hotkeys or mouse wheel or tab order?

[ivan-mogilko]

Please clarify, what is "regular Windows control", which WinForms class are you refering to?

[ivan-mogilko]

Anyway, I suppose that ToolStrip is used, because this drop-down must have number of buttons per each item.
Right now it has 2 buttons for "visible" and "locked" checkboxes, along with the item itself.

This is done by generating a custom ToolStripItem. I do not know if "standard" dropdown menu control can do the same.

[ericoporto]

I have a keyboard navigation too if this is something that is wanted, it's here: ericoporto/ags-old@9f6a129

[ivan-mogilko]

TBH I begin to think that I don't want to work on this control at all.
It may be not the best thing on its own, and may simply be unfinished, not having all the necessary functionality. Also, I cannot tell which parts of the code is original, and which added or modified by tzachs.

Perhaps it has to be replaced by a better control, properly written and with all mouse and keyboard input features.
Or replaced by a completely different way of navigating the room (something tree-like, as suggested on forums).

[Crystal-Shard]

Right now it has 2 buttons for "visible" and "locked" checkboxes, along with the item itself.

I'm really not sure why I (or anyone) would want to set individual walkbehinds/regions/walkables to visible or to locked.

Locking objects, I understand; because sometimes objects overlap and you'll want to move one but not the other. But this doesn't seem to apply to any of the layer types. And I'd say that whether something is "locked" should be a property on the property panel (just like e.g. an object's X, Y position) and not part of a pulldown.

[ivan-mogilko]

I'm really not sure why I (or anyone) would want to set individual walkbehinds/regions/walkables to visible or to locked.

Of course this was not meant for regions. It's simply that same control is used for everything.

Besides this, there were plans to add more controls on this dropdown, when we were planning to make Add/Remove of areas, to have a dynamic list of them, up to 256 items.

And I'd say that whether something is "locked" should be a property on the property panel (just like e.g. an object's X, Y position) and not part of a pulldown.

I decided that it's more convenient to lock there, and because it's not a part of the object's own data, and should not be saved as a part of game data either.
This functionality was moved to this bar on purpose, and existing "Locked" property that Objects had deprecated.

Same for "Visible", making things visible and not in the editor is not the same as Visible property, which means the starting object's state.

Locking and changing visibility in the editor is now saved as a part of "workspace" user data.

[Crystal-Shard]

Of course this was not meant for regions. It's simply that same control is used for everything.

Yes, and my point is that it probably shouldn't be.

Locking and changing visibility in the editor is now saved as a part of "workspace" user data.

That's the correct approach from a class/data handling perspective, but not necessarily from a UX perspective.

[ivan-mogilko]

I am not a interface designer. I asked someone to create a good control to edit rooms. This is the control that was created. There was nothing else done in years, and people were unhappy with a small drop-down combobox for selecting a layer, and no way to quickly select items, so I added that new control.

I tried to have things consistent with it, so modified object properties according to my perspective. It felt logical at the time.

If the new controls are wanted, I may suggest to write a specification, how it should look like, and how it should act.
Then somebody else, who has got time and ability, could develop them.

There have been this thread opened on forums:
https://www.adventuregamestudio.co.uk/forums/editor-development/suggestion-room-explorer-a-tree-like-control-for-navigating-room-contents/
this is a general suggestion, but it does not have to be followed. There may be other ideas.

[ivan-mogilko]

@ericoporto

The mouse wheel fix I tried is this one : https://github.com/Orbmu2k/nvidiaProfileInspector/blob/master/nspector/Common/Helper/DropDownMenuScrollWheelHandler.cs

Alright, so, using this reflection hack to get private "ScrollInternal" method actually works, and lets to scroll using mouse wheel event, at least in theory. I'd like to try to make a minimal possible adjustment using this method.

Another thing that may be trivial to do is to give this dropdown more height, at least enough to display first 16 items: that will at let see all walkable areas at once, for instance.

In regards to the whole situation around this control, I'd rather not discuss its replacement or redesign here at all; and only talk about possible minor improvements.

[ericoporto]

That code from Nvidia is pulled from SO here : https://stackoverflow.com/questions/13139074/mouse-wheel-scrolling-toolstrip-menu-items.

If you search for this Winforms control in GitHub and filter for C# you will find quite a few projects using variants of this hack and other hacks for the button, all apparently copy-pasted one from the other.

The issue I had at the time is the mouse over was only detected when the mouse was over the labels on the left and not either the buttons or the empty space on the right. I tried a few ideas and didn't figure a solution but perhaps I was doing something wrong. I burned an entire weekend on this though.

[ivan-mogilko]

Well, I put this code into existing ScrollDropDownMenu method, and it looks working so far.
Idk if I'm missing anything.
I'll make a PR for a test.

[ivan-mogilko]

In regards to the arrow buttons, that code from the pastebin gave me a curious idea.

If you cast ToolStripDropDownButton to Control, then you have access to Controls collection. This collection has two special controls of type "StickyLabel", which is an internal class it seems, but inherits Label.

So you can do:

Control tsControl = (tsDropDown as Control);
foreach (Control ctrl in tsControl.Controls)
{
    if (ctrl is Label)
    {
        ctrl.BackColor = Color.Red;
    }
}

Which turns these arrow buttons red.
Unfortunately, setting their Height directly did not work, but maybe something else will...

[ivan-mogilko]

Hah...

Control tsControl = (tsDropDown as Control);
foreach (Control ctrl in tsControl.Controls)
{
    if (ctrl is Label)
    {
        ctrl.BackColor = Color.Red;
        ctrl.MinimumSize = new Size(0, 30);
    }
}

The problem now is that the item arrangement does not change, so they begin to overlap with these buttons.
But this may be a matter of hacking this control's internals more...

[ericoporto]

I think I found it's source on google - not sure if the correct one as Microsoft has two Winforms... https://referencesource.microsoft.com/#system.windows.forms/winforms/managed/system/winforms/ToolStripScrollButton.cs,42

Check the comment in DefaultMargin. But I have no idea how to do that in someone's else internal protected stuff ....

The problem now is that the item arrangement does not change

Other possibility is to try to tell it to recalculate the layout.

[ivan-mogilko]

Ok, I solved this by doing an initial scroll in "Opened" event.

I'm currently trying to make the whole dropdown height bigger, to let it display a minimum of 16 items.

[ivan-mogilko]

Something I also looked into, is the problem where dropdown menu hides if you hover other bar items. This was bugging me for a long time.

I had to check the callstack, and search through the ToolStrip source code (linked above) to understand what is happening.
Apparently, this is because of "hot tracking", where moving mouse over an item automatically changes its "selected" state, so long as it is Enabled. When another item becomes "selected", any dropdown menu gets hidden.
I have not yet found any way to prevent this using only available public methods or properties.

Dropdown has a "Closing" event, which lets to interrupt its closing, but for some weird reason the "CloseReason" in this case is reported as "AppFocusChange"... idk if that's a mistake, or some kind of a default value. If I ignore that unconditionally, then the menu will stay on screen after alt+tabbing to another application. So, if using this approach, then there has to be additional condition to not close.

EDIT: okay, I found at least 1 way: combine the above with checking if anything in ToolStrip is selected.
perhaps there's a better and faster method, but I don't know it yet.

[ivan-mogilko]

@Crystal-Shard if you're interested in testing these experimental fixes, there's a editor build available for download:
https://cirrus-ci.com/task/5468321041088512

[ivan-mogilko]

Updated build for testing:
https://cirrus-ci.com/task/4945972789248000

[Crystal-Shard]

Ok, the pulldown now shows all walkbehinds / walkables / regions, which is a definite improvement.

Regarding hotspots, the mousewheel kind-of works, but it tears up the graphic and it scrolls too slowly (pretty much nothing in Windows scrolls only a single line for each "tick" of the wheel; it looks like other places in AGS default to three lines per "tick").

Also, I was thinking that the hotspot pulldown in particular would be much more useful if it shows the description of the hotspot, instead of showing in parentheses that it's a hotspot. Currently it says "hHotspot6 (Hotspot; ID 6)", it would be nice to change to e.g. "hHotspot6 (door; ID 6)". I'm mentioning this because many games don't change the name of each hotspot but they do change the description so that mouseover text works. This applies to the pulldown in the properties panel, too.

[ericoporto]

I think the scroll speed may be a bit mouse dependant - I have one of those that you can roll with inertia so I didn't notice it was slow, but not being able to scroll one tick would be noticeable because ideally if you scroll the minor amount it should advance/reverse only one item.

I added a fix for the tear in the PR as a comment.

Also not renaming the hotspot looks like a bad coding practice, if they weren't statically defined I would expect they to just be created with NewHotspot as their name...

[Crystal-Shard]

Also not renaming the hotspot looks like a bad coding practice, if they weren't statically defined I would expect they to just be created with NewHotspot as their name...

I'm talking about the description of the hotspot, not the name. Hotspots are created with a blank description and a generic name like hHotspot6.

[ericoporto]

That name is meant to be changed, in your example it would be hDoor, otherwise that wouldn't be there and the global array would be sufficient (hotspot[6]). You want your code to make sense and feel like written text.

Edit: btw I don't even use descriptions... But some games I know use the description to write the look at - I think A Landlords Dream did it first.

[Crystal-Shard]

That name is meant to be changed, in your example it would be hDoor, otherwise that wouldn't be there and the global array would be sufficient (hotspot[6]). You want your code to make sense and feel like written text.

That description is also meant to be changed, and meant to be descriptive. That's why it makes sense to include both name and description in the pulldown.

Edit: btw I don't even use descriptions... But some games I know use the description to write the look at - I think A Landlords Dream did it first.

ATOTK did that eight years earlier :P

[ivan-mogilko]

A lot of games use descriptions for the purpose of displaying on a label. That's the reason why users requested to be able to modify this description at runtime too.

Anyway, if I change the item title, it's probably better to do this consistently for everything.

Last time I looked into this code, there was some issue with this displayed name connected with the name displayed on a Property Grid's list. I must review this and double check that it is not used in any key matching first...

---

I also did not notice that scrolling is slow, this may be mouse dependant, as eri0o sais, maybe there's a standard solution for bringing this to some uniformity, or this could be solved by adding an editor preference...

[ivan-mogilko]

Ah, but I don't think I should be changing these item texts within same task. Here I'd like to focus on these limited changes to navigation. Other suggestions should better be recorded separately.

[ivan-mogilko]

Forgot to post this earlier, this is the recent variant:
https://cirrus-ci.com/task/6313599663079424
with smoother render, and possibly fixed scrolling speed.

[ivan-mogilko]

@Crystal-Shard could you tell if the latest variant (posted above) works better in terms of scrolling for you?

[Crystal-Shard]

I'll check, but I'm traveling this week so it'll have to wait until I'm back home.

[ivan-mogilko]

I merged the changes, because the solution works overall; any additional adjustments could be done on top of this.