Skip to content

Better Filter on Path (Text, Type and Property)#107

Closed
53845714nF wants to merge 5 commits intofaaxm:masterfrom
53845714nF:add_text_type_finding
Closed

Better Filter on Path (Text, Type and Property)#107
53845714nF wants to merge 5 commits intofaaxm:masterfrom
53845714nF:add_text_type_finding

Conversation

@53845714nF
Copy link
Contributor

@53845714nF 53845714nF commented Mar 19, 2024

I added the option in Path to use # for finding types of a Qml Object.
Finding Text with "yout text". And a filter for looking at Properties (source=qrc:/menu/back.png).

Example in Python:

from xmlrpc.client import ServerProxy

# Connect to the application
session = ServerProxy('http://192.168.178.142:9000')

# Looking for Type
session.existsAndVisible('mainWindow/#Rectangle')

# Looking for Text
session.existsAndVisible('mainWindow/"Click Me"')

This can then be used in all spix functions.

For the Property Filter, I use () so select them. They also must have a = to look what is the Value and what is the Property.

session.mouseClick('mainWindow/(source=qrc:/menu/back.png)')

It is very cool when you use this on a back arrow Button.

@53845714nF 53845714nF mentioned this pull request Apr 17, 2024
@53845714nF 53845714nF force-pushed the add_text_type_finding branch from 477aa6b to d918a14 Compare April 26, 2024 10:28
@53845714nF 53845714nF changed the title Add finding text and type in the normal Path Better Filter on Path Apr 26, 2024
@53845714nF 53845714nF changed the title Better Filter on Path Better Filter on Path (Text, Type and Property) Apr 26, 2024
@faaxm faaxm closed this Mar 24, 2025
@faaxm faaxm reopened this Mar 24, 2025
@faaxm
Copy link
Owner

faaxm commented Apr 3, 2025

close and re-open to trigger fresh build

@faaxm faaxm closed this Apr 3, 2025
@faaxm faaxm reopened this Apr 3, 2025
@chsterz chsterz force-pushed the add_text_type_finding branch from f80008f to 4d4cc45 Compare April 16, 2025 11:29
@chsterz
Copy link

chsterz commented Apr 16, 2025

@faaxm I think you need to poke gh again :)

@chsterz chsterz force-pushed the add_text_type_finding branch from 4d4cc45 to a40e0b7 Compare April 16, 2025 13:11
@chsterz chsterz force-pushed the add_text_type_finding branch from a40e0b7 to 3f095d1 Compare April 16, 2025 14:33
@chsterz
Copy link

chsterz commented Apr 16, 2025

@faaxm sorry, my clangformat does run very selectively apparently, please recheck again :)
I had to repoke it to also remove double lines, etc.
Maybe later we can add pre-commit hooks for clangformat to exclude that issue early on
Edit: I have seen that theres a PR for that ❤️

Copy link
Owner

@faaxm faaxm left a comment

Choose a reason for hiding this comment

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

I like the general idea, and had a few comments regarding some details.

One thing I noticed is that the functions for parsing the item name/path component in the QtScene and the ones for searching items have become rather long and a bit less obvious, so I am wondering if it would make sense to refactor this a bit.

However, I am also wondering how this best fits with opening Spix up towards other UI Frameworks. Moving the path parsing into the ItemPath object might be a solution as all Scenes could then use the same logic... Maybe with ItemPath having a list of ItemPathComponents, rather than plain strings

Having a common path with filtering support however begs the question if there is any sensible syntax that makes sense for all ui frameworks...

So... basically just thinking out loud here as I am not quite sure about this myself yet... Would be interested in your thoughts and might play around with some ideas myself during the next week. Still wanted to provide some initial feedback though :)

Comment on lines +27 to +39
if (pathss.peek() == '\"') {
getline(pathss, component, '\"');
getline(pathss, component, '\"');
component = '\"' + component + '\"';
m_components.push_back(std::move(component));
}

if (pathss.peek() == '(') {
getline(pathss, component, '(');
getline(pathss, component, ')');
component = '(' + component + ')';
m_components.push_back(std::move(component));
}
Copy link
Owner

Choose a reason for hiding this comment

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

Is the purpose of handling components that start with " or ( separately to make sure that intermittent '/' are kept as part of the component? Because if that is the only reason and I am not missing anything, I think it would make sense to combine that with a fix to issue #45 and to have a more generic '' way of escaping a '/'.
Otherwise it feels a bit like part of the logic for parsing special path components is distributed among the ItemPath class and the QtScene implementation.

Also, a path "/foo/"bar"(baz)/x" would now be the same as "/foo/"bar"/(baz)/x" which is a bit confusing. Probably would also make sense to extend the unit test for the ItemPath to cover those extra cases...

I know this is an old PR, so I wouldn't mind joining in the effort and fixing this myself. Just let me know if I missed anything when looking at this ;-)

Comment on lines +78 to +79
size_t found = itemName.find("\"");
auto searchText = itemName.substr(found + 1, itemName.length() - 2);
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
size_t found = itemName.find("\"");
auto searchText = itemName.substr(found + 1, itemName.length() - 2);
auto searchText = itemName.substr(1, itemName.length() - 2);

should be good enough since we already know the " is the first character.

Comment on lines +86 to +87
size_t found = itemName.find("#");
auto type = QString::fromStdString(itemName.substr(found + 1));
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
size_t found = itemName.find("#");
auto type = QString::fromStdString(itemName.substr(found + 1));
auto type = QString::fromStdString(itemName.substr(1));

(as above)

}
}

if (GetObjectName(child) == name) {
Copy link
Owner

Choose a reason for hiding this comment

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

isn't this obsolete now that the name is compared in the if (name.has_value()) clause above?

Comment on lines +136 to +138
if (auto item = FindChildItem(child, name, {}, {}, type)) {
return item;
}
Copy link
Owner

Choose a reason for hiding this comment

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

I'm wondering if a depth-first search is still the right approach when searching for something by type. With object names, it is more safe to assume that they are somewhat unique, but if I search for a Text in a root-view, I would probably assume to get the first text in that root view, and not the Text object that is a child of a child of the first Rectangle in that root view...

Comment on lines +171 to +176
QString getNameForObject(QObject* object)
{
QString name = "#" + spix::qt::TypeByObject(object);

return name;
}
Copy link
Owner

Choose a reason for hiding this comment

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

I think this function is not used anywhere. Is it a leftover from debugging?

Comment on lines +22 to +36
QString getNameForObject(QObject* object)
{
QString name;
if (spix::qt::PropertValueByObject(object, QString::fromStdString("text")) != "") {
name = "\"" + spix::qt::PropertValueByObject(object, QString::fromStdString("text")) + "\"";
} else if (spix::qt::PropertValueByObject(object, QString::fromStdString("source")) != "") {
name = "(source=" + spix::qt::PropertValueByObject(object, QString::fromStdString("source")) + ")";
} else if (spix::qt::GetObjectName(object) != "") {
name = spix::qt::GetObjectName(object);
} else {
name = "#" + spix::qt::TypeByObject(object);
}

return name;
}
Copy link
Owner

Choose a reason for hiding this comment

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

I think this function is not used anywhere. Is it a leftover from debugging?

Comment on lines +104 to +105
size_t foundBracketSign = itemName.find('(');
auto searchText = itemName.substr(foundBracketSign + 1, itemName.length() - 2);
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
size_t foundBracketSign = itemName.find('(');
auto searchText = itemName.substr(foundBracketSign + 1, itemName.length() - 2);
auto searchText = itemName.substr(1, itemName.length() - 2);

Comment on lines +152 to 164
if (propertyValue.has_value() && propertyName.has_value()) {
if (auto item = FindChildItem(child, name, propertyName.value(), propertyValue.value(), {})) {
return item;
}
} else if (type.has_value()) {
if (auto item = FindChildItem(child, name, {}, {}, type)) {
return item;
}
} else {
if (auto item = FindChildItem(child, name)) {
return item;
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

I think this could just be a single

Suggested change
if (propertyValue.has_value() && propertyName.has_value()) {
if (auto item = FindChildItem(child, name, propertyName.value(), propertyValue.value(), {})) {
return item;
}
} else if (type.has_value()) {
if (auto item = FindChildItem(child, name, {}, {}, type)) {
return item;
}
} else {
if (auto item = FindChildItem(child, name)) {
return item;
}
}
if (auto item = FindChildItem(child, name, propertyName, propertyValue, type)) {
return item;
}

Where the optionals are just passed along, leaving the has_value() check to the FindChildItem call.

}

QObject* FindChildItem(QObject* object, const QString& name)
QString PropertValueByObject(QObject* object, QString propertyName)
Copy link
Owner

Choose a reason for hiding this comment

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

typo

Suggested change
QString PropertValueByObject(QObject* object, QString propertyName)
QString PropertyValueByObject(QObject* object, QString propertyName)

@faaxm
Copy link
Owner

faaxm commented Apr 26, 2025

Hi @53845714nF and @chsterz !

Your PR made me review how items were searched previously and how paths were managed in general. Once I started restructuring things, this ended up being a bigger change, but with the new system in place, adding new selectors was quite straight forward. I cherry-picked some of your changes into a new branch with the new path system. Hope you don't mind.

I will leave those PRs open during the next week, so if you want to have a look or propose changes (in case I missed something that was important for your particular use case), let me know and we can discuss how to best merge your changes.

The PR with the general changes of how paths work: #136
The PR with the new selectors: #137 (also adds some documentation)

@53845714nF
Copy link
Contributor Author

53845714nF commented Apr 28, 2025

Hello,

I think this is a good way to go. I can no longer answer all the questions ad hoc about what I did a year ago.
Since you are the code maintainer, I think it is best to use your version.

@faaxm
Copy link
Owner

faaxm commented May 1, 2025

Closing since alternative PRs have been merged. (#136, #137)

@faaxm faaxm closed this May 1, 2025
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.

3 participants