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

Basic support for multiple cars. #39

Merged
merged 1 commit into from
Aug 9, 2024
Merged

Basic support for multiple cars. #39

merged 1 commit into from
Aug 9, 2024

Conversation

flosoft
Copy link
Owner

@flosoft flosoft commented Aug 5, 2024

Initial revision to adding support for multiple cars as per issue #38 .

@Zegorax, as you refactored quite a bit of the backend, I was wondering if you could have a look and check whether this approach makes sense to you.

@Zegorax
Copy link
Contributor

Zegorax commented Aug 6, 2024

I will take a look today or tomorrow

@Zegorax
Copy link
Contributor

Zegorax commented Aug 6, 2024

In the end I've just checked it out now :)

So, originally I wanted to submit a third PR to refactor a little bit the UI part and properly separate the frontend / backend parts. My goal would be to avoid generating the jinja2 template with hardcoded values from the backend, and instead serve a raw HTML page or a small Angular app which will then query the API with what it needs (car config, periodic map update fetch, etc)

Regarding this specific PR, I would suggest to remove the "MULTICAR_SUPPORT" and instead allow all links to be linked to a car id, and if there is only one, use it as default.

The backend would fetch the number of cars and then the frontend would allow to choose one for each link.

What do you think ? I will try to draft a PR tomorrow with my thoughts.

@flosoft
Copy link
Owner Author

flosoft commented Aug 6, 2024

The backend would fetch the number of cars and then the frontend would allow to choose one for each link.

I had a look on the endpoints in TeslaLogger, but it doesn't have any easily accessible endpoint that lists all available carid's, short of just testing every single ID sequentially (which has it's downsides too).

There may also be people who don't want to allow generating links for all available cars and instead just limit it to a specific one.

@Zegorax
Copy link
Contributor

Zegorax commented Aug 6, 2024

Allow me to rephrase it. I think it's better to have a selector with which you can select the desired car when creating a link. It would also be a nicer touch in the UI

@flosoft flosoft merged commit 822370d into master Aug 9, 2024
@Zegorax
Copy link
Contributor

Zegorax commented Aug 11, 2024

Just FYI, this PR broke everything on my install, I had to revert it on my repo.
I think it merits a better implementation with a proper selection in the UI, which proposes vehicles based on what the provider has.

new_car_id = result.carid
else:
new_car_id = BACKEND_PROVIDER_CAR_ID
BackendProviderFactory.provider.car_id = new_car_id
Copy link
Contributor

Choose a reason for hiding this comment

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

This will not work because the backend does not provide any "car_id" entity at the runtime, it is only used for the construction of the GET request URL.

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.

2 participants