Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix PgUp/PgDown not changing structure palette #1445

Conversation

alynsarcana
Copy link
Contributor

@alynsarcana alynsarcana commented Mar 12, 2024

The number keys called a local function that handled changing the structure picker. This change adds a 2nd parameter (defaulting to null) to that local function, allowing the event handlers for PageUp/Down to call the same function, rather than call directly to the mMapView object. If the 2nd parameter have a value, then the 1st parameter is ignored and the 2nd is used to change the mMapView z level.


Edit:
Closes #1364

@DanRStevens DanRStevens changed the title Initial attempt to address issue #1364 Fix PgUp/PgDown not changing structure palette Mar 12, 2024
@DanRStevens
Copy link
Member

Oh interesting, and unexpected.

I edited the lead comment to link to the issue. Using the lead message with closes allows the issue to auto close when the PR is merged. I also adjusted the title. I figure a description is more useful in the title for browsing than an issue number. Plus, as I've just discovered, the issue doesn't get linked when referenced from the title, but does get linked when referenced in comments.

Refactor (+1 squashed commits)

Squashed commits:

[972b4f76] Remove second default parameter in favor of calculating new depth in button press event handler.
DanRStevens
DanRStevens previously approved these changes Mar 19, 2024
Copy link
Member

@DanRStevens DanRStevens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. One minor spacing inconsistency I noticed.

I'll push the branch to the repo so the tests run.

@DanRStevens
Copy link
Member

Aha, I've discovered how to fetch the changes without having to first add a new remote for the fork. Seems there are some GitHub refs that can be used, which don't normally appear in branch lists.

For example:

git fetch origin pull/1445/head:fixPgUpPgDown
git co fixPgUpPgDown
git push -u origin fixPgUpPgDown

That's quite a bit more convenient than adding a new remote.

Anyway, tests are running. The Windows build can take somewhere from 5-15 minutes. I won't be sticking around though. Maybe the whitespace thing will be fixed before I revisit.

@DanRStevens
Copy link
Member

When testing this locally I noticed one oddity, which I haven't really looked into yet. The hotkeys all work with these changes, but the mouse buttons no longer switch the construction menu. Checking against main, it seems the mouse buttons did switch the construction menu before.

It's curious because there were no obvious changes to the mouse button handling code. Maybe there were extra calls to handle the level change side effects which are no longer getting called due to the z-level change detection? I know a few places in the game checked certain conditions every frame drawn, not simply when a state change was made.

@DanRStevens
Copy link
Member

In MapViewState::onMouseDown, there is a call to changeViewDepth, which should be updated to call onChangeDepth instead:

		const auto oldDepth = mMapView->currentDepth();
		mNavControl->onClick(MOUSE_COORDS);
		if (oldDepth != mMapView->currentDepth())
		{
			changeViewDepth(mMapView->currentDepth());
		}

@DanRStevens DanRStevens dismissed their stale review March 27, 2024 18:34

Found a bug. See comment.

Copy link
Member

@DanRStevens DanRStevens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent. I just ran a manual test and this fixes the mouse button issue I mentioned earlier.

@DanRStevens DanRStevens merged commit 0a22150 into OutpostUniverse:main Apr 13, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PageUp/Down key from underground to surface doesn't change structure picker palette
3 participants