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

Preserved selector #34105

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Preserved selector #34105

wants to merge 11 commits into from

Conversation

pbassut
Copy link
Contributor

@pbassut pbassut commented Nov 25, 2024

Sometimes you use your preserved routes as "favorites". So having a tab in cabana to only show those you're interested in seems like a reasonable feature.

@github-actions github-actions bot added the tools label Nov 25, 2024
@pbassut
Copy link
Contributor Author

pbassut commented Nov 25, 2024

image

@deanlee
Copy link
Contributor

deanlee commented Nov 26, 2024

Thank you for the contribution! The code works well, although it feels a bit complex. I believe we could simplify it by using a common route_list_ for both preserved and all routes, potentially achieving the same functionality with fewer than 20 additional lines of code.

Additionally, I noticed some UI issues with the route list, such as an odd border on Linux and the route_list_ does not stretch to the full height when the window is resized by dragging(this is an old bug). I can handle fixing these issues and the code cleanup after the merge.

2024-11-26_14-20

@pbassut
Copy link
Contributor Author

pbassut commented Nov 26, 2024

The old bug one i would rather handle them in a separate issue since they're an old thing.
As for using a common route_list widget, in QT can we have the same widget be shared in two different windows?

@deanlee
Copy link
Contributor

deanlee commented Nov 26, 2024

Yes, you will need to re-parent it after switching tabs. Actually, forget what I said earlier—having two separate route lists works just fine when using tabs.

@pbassut
Copy link
Contributor Author

pbassut commented Nov 26, 2024

@deanlee let me know if there are any code tweaks you would like me to do to make this simpler.
Otherwise, what's left to merge this in?

@pbassut
Copy link
Contributor Author

pbassut commented Nov 26, 2024

Give me a minute so I can experiment with a code change here

@pbassut pbassut marked this pull request as draft November 26, 2024 17:15
@@ -18,9 +21,14 @@ class RoutesDialog : public QDialog {
void parseDeviceList(const QString &json, bool success, QNetworkReply::NetworkError err);
void parseRouteList(const QString &json, bool success, QNetworkReply::NetworkError err);
void fetchRoutes();
bool isPreservedTabSelected();

Copy link
Contributor

@deanlee deanlee Nov 26, 2024

Choose a reason for hiding this comment

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

remove blank line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The blank here is intentional to separate methods from instance variables. But I can remove it if you feel strongly about this

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with it

@pbassut pbassut marked this pull request as ready for review November 26, 2024 22:55
@pbassut pbassut requested a review from deanlee November 26, 2024 22:55
@deanlee
Copy link
Contributor

deanlee commented Nov 27, 2024

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants