Skip to content

MapFindFeature: Exclude hidden and protected objects - #2371

Merged
dg0yt merged 16 commits into
OpenOrienteering:masterfrom
dl3sdo:fix-find-feature
May 17, 2025
Merged

MapFindFeature: Exclude hidden and protected objects#2371
dg0yt merged 16 commits into
OpenOrienteering:masterfrom
dl3sdo:fix-find-feature

Conversation

@dl3sdo

@dl3sdodl3sdo commented Apr 27, 2025

Copy link
Copy Markdown
Member

The 'Find next' search failed if the object query returned an object with either a hidden or protected symbol since these kind of objects can't be added to an object selection.
Exclude all hidden and protected objects from the 'Find next' and 'Find all' search.

@dl3sdo
MapFindFeature: Exclude hidden and protected objects
a8195c6
The 'Find next' search failed if the object query returned an object
with either a hidden or protected symbol since these kind of objects
can't be added to an object selection.
Exclude all hidden and protected objects from the 'Find next' and
'Find all' search.
@dg0yt
dg0yt self-requested a review April 27, 2025 13:24
dg0yt
dg0yt reviewed Apr 27, 2025
View reviewed changes
Comment threadsrc/gui/map/map_find_feature.cpp Outdated
@dl3sdo

dl3sdo commented Apr 27, 2025

Copy link
Copy Markdown
MemberAuthor

For the 'Find all' mechanism the hidden and protected objects were just ignored when adding to the object selection, whereas for the 'Find next' mechanism the outcome depended on the internal order of the objects:

FindObjects_Issue2371.mp4

@dg0yt

dg0yt commented Apr 27, 2025

Copy link
Copy Markdown
Member

Yeah, I got the problem. I'm a little bit slow with reviewing the changes 🙈

@dg0ytdg0yt added this to the v0.9.6 milestone May 4, 2025
@dg0ytdg0yt self-assigned this May 4, 2025
@dg0yt

dg0yt commented May 12, 2025•
edited
Loading

Copy link
Copy Markdown
Member

Made findNext and findAll more similar, both using applyOnMatchingObjects(..., query) now. Avoiding the second walk for findNext.

It is a little bit irritating that objects with protected symbols are skipped. But this can't be changed easily now. The PR is an improvements nevertheless.

dg0yt
dg0yt reviewed May 12, 2025
View reviewed changes
Comment threadsrc/gui/map/map_find_feature.cpp Outdated
@dl3sdo

dl3sdo commented May 12, 2025

Copy link
Copy Markdown
MemberAuthor

@dg0yt: adding the std::cref around query again fixes the compiler issue for me (it's compiling however). CI stops for some targets.

dg0yt
dg0yt reviewed May 13, 2025
View reviewed changes
Comment threadsrc/gui/map/map_find_feature.cpp Outdated
}
};
map->getCurrentPart()->applyOnMatchingObjects(search, std::cref(query));
map->getCurrentPart()->applyOnAllObjects(search);

@dg0ytdg0ytMay 13, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reverting part of my change here: The current object might no longer match the query, but we still want to see it when determining the next match.

@dl3sdo

dl3sdo commented May 13, 2025

Copy link
Copy Markdown
MemberAuthor

@dg0yt: I'm adding a test case.

@dg0yt

dg0yt commented May 13, 2025

Copy link
Copy Markdown
Member

I will probably revert most of b0dcb5a. const and slot were omitted intentionally.

@dl3sdo

dl3sdo commented May 13, 2025

Copy link
Copy Markdown
MemberAuthor

I decided to not create another separate file but to add the test case to ObjectQueryTest.
It took me some time to find out that I could not make ObjectQueryTest a friend of MapFindFeature since ObjectQueryTest is in a different namespace. And changing the namespace will cause trouble with Q_INIT_RESOURCE(resources); doStaticInitializations();
I am surprised that the search operates in reverse order (this was already the situation before this PR).

dg0yt added 2 commits May 14, 2025 14:33
@dg0yt
Move and revise testFindFeatures
b401a03
@dg0yt
Partially revert polishing
b223b89
No slot markup, member functions are good enough.
No const on functions which hand out non-const member.
Restore original order in header which matched order in source.
Move the new functions for testing purpose from public to protected.
The test can access them via light-weight derived type.
@dg0yt
dg0ytforce-pushed the fix-find-feature branch from d5c18d7 to b223b89CompareMay 14, 2025 12:37
@dl3sdo

dl3sdo commented May 14, 2025

Copy link
Copy Markdown
MemberAuthor

Thank you Kai for refactoring my test code! BTW: tools_t.cpp was the file I also had in mind to put the test in and from which I tried to take over some code.

@dg0yt

dg0yt commented May 14, 2025

Copy link
Copy Markdown
Member

The last commit is the result of being forced into refactoring...

We can make the testable functions public because they only need the controller and query, not the object state. (They might be member functions of controller, but you know ... I don't want to add more capabilities to that class.)

@dl3sdo

dl3sdo commented May 14, 2025

Copy link
Copy Markdown
MemberAuthor

@dg0yt: I checked the code once more and everything looks good to me. I really like the clear structure and especially the search functor which does now everything in just one function! Thanks again.

@dg0yt

dg0yt commented May 14, 2025

Copy link
Copy Markdown
Member

A few more twists.
It is really good now ;-)

@dl3sdo

dl3sdo commented May 17, 2025•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

I tested the final version and reviewed the code once again, everything now really looks good and works as intended. Thank you Kai for spending the effort.
Rebasing #2374 upon that PR will be really fun ;-)
Not related to this PR: I wonder if anyone really uses the 'Find next' entry in the 'Edit' menu instead of doing the search in the 'Find...' dialog.

@dg0yt

dg0yt commented May 17, 2025

Copy link
Copy Markdown
Member

Not related to this PR: I wonder if anyone really uses the 'Find next' entry in the 'Edit' menu instead of doing the search in the 'Find...' dialog.

This entry is for searching with the dialog hiding parts of the map. There used to be also shortcuts IIRC.

@dg0yt
dg0yt merged commit 7dc7cfe into OpenOrienteering:masterMay 17, 2025
@dg0yt

dg0yt commented May 17, 2025

Copy link
Copy Markdown
Member

Thanks Matthias.

@dl3sdo
dl3sdo deleted the fix-find-feature branch May 17, 2025 18:24
dl3sdo added a commit to dl3sdo/mapper that referenced this pull request May 17, 2025
@dl3sdo
Adjustments after rebasing onto master due toOpenOrienteeringGH-2371
adf7940
dl3sdo added a commit to dl3sdo/mapper that referenced this pull request Sep 3, 2025
@dl3sdo
Adjustments after rebasing onto master due toOpenOrienteeringGH-2371
91b38c6
dl3sdo added a commit to dl3sdo/mapper that referenced this pull request Dec 31, 2025
@dl3sdo
Adjustments after rebasing onto master due toOpenOrienteeringGH-2371
3535eaa
dl3sdo added a commit to dl3sdo/mapper that referenced this pull request Jan 7, 2026
@dl3sdo
Adjustments after rebasing onto master due toOpenOrienteeringGH-2371
eb71a27
dl3sdo added a commit to dl3sdo/mapper that referenced this pull request Feb 28, 2026
@dl3sdo
Adjustments after rebasing onto master due toOpenOrienteeringGH-2371
4d1496d
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dg0ytdg0ytdg0yt left review comments

Assignees

@dg0ytdg0yt

Labels

None yet

Projects

None yet

Milestone

v0.9.6

Development

Successfully merging this pull request may close these issues.

2 participants

@dl3sdo@dg0yt