Add Jump to Tile Coordinates feature#4215
Add Jump to Tile Coordinates feature#4215sirajahmadzai wants to merge 1 commit intomapeditor:masterfrom
Conversation
- Implement TileLocatorSource for parsing tile coordinates - Add 'Jump to Tile' action with Ctrl+Shift+G shortcut - Support multiple coordinate formats: x,y, x:10 y:20, 10 20, etc. - Update keyboard shortcuts documentation - Fixes issue mapeditor#1367
sirajahmadzai
force-pushed
the
feature/jump-to-tile-coordinates
branch
from
08afd98 to
3e6b93e
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds a "Jump to Tile Coordinates" feature that allows users to navigate directly to specific tile locations using keyboard input. The implementation provides a locator widget that accepts multiple coordinate formats and integrates with the existing locator system.
Key changes:
- Implements TileLocatorSource for parsing and handling tile coordinate input
- Adds jump to tile action with Ctrl+Shift+G keyboard shortcut
- Supports multiple coordinate formats (x,y / x:10 y:20 / 10 20 / single numbers)
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tiled/tilelocator.h | New header defining TileLocatorSource class and Match structure |
| src/tiled/tilelocator.cpp | Implementation of coordinate parsing, UI delegate, and navigation logic |
| src/tiled/mainwindow.ui | Added Jump to Tile action and menu item with Ctrl+Shift+G shortcut |
| src/tiled/mainwindow.h | Added jumpToTile method declaration |
| src/tiled/mainwindow.cpp | Connected action to jumpToTile method implementation |
| src/tiled/libtilededitor.qbs | Added new source files to build system |
| docs/manual/keyboard-shortcuts.rst | Updated documentation with new keyboard shortcut |
| return true; | ||
| } | ||
|
|
||
| // Format 2: "x:10 y:20" or "x:10, y:20" |
There was a problem hiding this comment.
The regular expression pattern is complex and hard to understand. Consider breaking it into smaller, more readable patterns or adding a comment explaining the expected format.
| // Format 2: "x:10 y:20" or "x:10, y:20" | |
| // Format 2: "x:10 y:20" or "x:10, y:20" | |
| // Matches formats like "x:10 y:20" or "x:10, y:20". | |
| // - "x:10" captures the x-coordinate. | |
| // - "y:20" (optional) captures the y-coordinate, separated by a comma or whitespace. |
There was a problem hiding this comment.
@bjorn this is not a regular expression, it is a comment
There was a problem hiding this comment.
Well, it is definitely talking about a regular expression, it just placed its comment off by one line. It appears to apply to the line below:
QRegularExpression xyPattern(QStringLiteral(R"(x\s*:\s*(\d+)(?:\s*[,\s]\s*y\s*:\s*(\d+))?)"));| QString remaining = text.mid(match.capturedEnd()); | ||
| QRegularExpression yPattern(QStringLiteral(R"(y\s*:\s*(\d+))")); | ||
| auto yMatch = yPattern.match(remaining); | ||
| if (yMatch.hasMatch()) { | ||
| tilePos.setY(yMatch.captured(1).toInt()); |
There was a problem hiding this comment.
[nitpick] The nested logic for handling partial x:y coordinate matches is complex and could be simplified. Consider extracting this into a separate helper method.
| QString remaining = text.mid(match.capturedEnd()); | |
| QRegularExpression yPattern(QStringLiteral(R"(y\s*:\s*(\d+))")); | |
| auto yMatch = yPattern.match(remaining); | |
| if (yMatch.hasMatch()) { | |
| tilePos.setY(yMatch.captured(1).toInt()); | |
| if (parseYCoordinateFromRemainingText(text.mid(match.capturedEnd()), tilePos)) { |
| // Try to find any open map document | ||
| const auto documents = documentManager->documents(); | ||
| for (auto document : documents) { | ||
| if (auto mapDoc = qobject_cast<MapDocument*>(document.data())) { | ||
| mapDocument = mapDoc; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
[nitpick] The fallback logic for finding a map document when no current document exists could be extracted into a helper method to improve readability.
| // Try to find any open map document | |
| const auto documents = documentManager->documents(); | |
| for (auto document : documents) { | |
| if (auto mapDoc = qobject_cast<MapDocument*>(document.data())) { | |
| mapDocument = mapDoc; | |
| break; | |
| } | |
| } | |
| mapDocument = findFallbackMapDocument(documentManager); |
bjorn
left a comment
bjorn
left a comment
There was a problem hiding this comment.
I normally don't click the "let AI review the code" button, but this PR seemed to be AI-written so I decided to let AI have a look at it. I'll see about giving it a proper look soon.
Did you use AI to code this, or is just the PR comment AI written?
| return true; | ||
| } | ||
|
|
||
| // Format 2: "x:10 y:20" or "x:10, y:20" |
There was a problem hiding this comment.
Well, it is definitely talking about a regular expression, it just placed its comment off by one line. It appears to apply to the line below:
QRegularExpression xyPattern(QStringLiteral(R"(x\s*:\s*(\d+)(?:\s*[,\s]\s*y\s*:\s*(\d+))?)"));2 participants
This PR implements the "Jump to location by tile coordinates" feature requested in issue #1367.
Changes:
Testing:
Files Changed:
src/tiled/tilelocator.h- New header filesrc/tiled/tilelocator.cpp- New implementationsrc/tiled/mainwindow.ui- Added action and menu itemsrc/tiled/mainwindow.h- Added method declarationsrc/tiled/mainwindow.cpp- Added method implementationsrc/tiled/libtilededitor.qbs- Added to build systemdocs/manual/keyboard-shortcuts.rst- Updated documentationFixes #1367