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

Adding Google Service has bug in callback for python3 branch #193

Open
dadr opened this issue Jun 12, 2021 · 9 comments
Open

Adding Google Service has bug in callback for python3 branch #193

dadr opened this issue Jun 12, 2021 · 9 comments

Comments

@dadr
Copy link
Contributor

dadr commented Jun 12, 2021

@Snille reported a bug in #189 that adding a google service was still issuing a callback to frameip:7777. I started a branch to fix that, and found a missed item in /extras/html/index.php. But that does not seem to do anything. I'm wondering if I just missed another :7777 somewhere, or whether some code on sensenet has that port hard-coded? If it's the latter, do we need to adjust flask to keep listening to :7777 in addition to port 80 or is it something that would be better to change at sensenet? Also, as I was changing the original :7777 references, I noticed what appeared to be hard-coded messages in the code. Do you think that is "bug worthy?" I don't think you will see the right addresses in on-screen-messages if you use the command line option to listen to a different port.

And @mrworf, while I'm typing, could you comment on whether I'm using GitHub as expected w/r/t pull requests? I find testing easier by doing PRs into my own python3 branch, but that seems to be creating a "moving target" PR into your branch. Is that OK? I fear I'm using the tool incorrectly because it seems to have gotten out of sync. For example, #192 lists a long litany of commits that I made after issuing #187. But in comparing the two current branches visually, I think that the later commits are actually already in your branch, and that #192 only changes Readme and adds the new commit for #189.

@JuliAn50M
Copy link

@dadr @mrworf it ll be great to fix this issue, please. Because with it we can't test and help you to validate your job :)

@dadr
Copy link
Contributor Author

dadr commented Jul 5, 2021

I'm still not sure how to fix that, but I have been able to work-around. When you get the bad URL you can edit it in the browser url line to remove the :7777 and then go to that link. That will register the google service and there are no issues after that point.

@JuliAn50M
Copy link

I'm still not sure how to fix that, but I have been able to work-around. When you get the bad URL you can edit it in the browser url line to remove the :7777 and then go to that link. That will register the google service and there are no issues after that point.

You are right. It works.

@mrworf
Copy link
Owner

mrworf commented Jul 6, 2021

This code https://github.com/mrworf/photoframe/blob/master/extras/html/index.php needs to be updated to support a non-7777 destination. To support both old an new the whole registration flow would need to change. Essentially the new version should pass the port as well as the IP to the service. The service should assume 7777 if no port is provided, so it can continue to work with the older frames.

@mrworf
Copy link
Owner

mrworf commented Jul 6, 2021

I'll see if I can do that change this weekend since I also would need to restart the service running on my server to enable it.

@dadr
Copy link
Contributor Author

dadr commented Jul 8, 2021

Thanks @mrworf ! I had good intentions to jump on the changes you requested, but I also have my Dad visiting for July, so I don't have as much free time as I normally do. Still, I've made some progress. If you're curious, it's the feature/backup branch on my clone at: https://github.com/dadr/photoframe Almost all the work (in progress) is in a new file: load_config.py I have all the basics that I did before, but I've also thought it would be prudent to call the same loading and checking of config values that the normal frame process does. My next step is to figure out what class to instantiate to get that done. I'm also still mulling over what to do if the value testing fails: revert, or warn. It seems that photo frame will fix bad values if it finds them. And then there is whether to test and what to do if the settings being loaded for screen resolution do not show up as a supported value for the current system. <Maybe I'm over-thinking it>

@mrworf
Copy link
Owner

mrworf commented Aug 6, 2021

Sorry for the delay, but backend now supports ports. Still need to edit the google service oauth bit, but should be done very soon.

Regarding time, I hear you and no rush, this should be done because we like to, not because it's our job ;-)

However, I think it's a bit dangerous to assume that the frame will always be able to auto correct. In some cases maybe that's not even what you want.

@mrworf
Copy link
Owner

mrworf commented Aug 6, 2021

@dadr @JuliAn50M the fix is in on the python3 branch, please verify and let me know if this solved usage of port 80

@dadr
Copy link
Contributor Author

dadr commented Aug 6, 2021

Thank you mrworf! Adding Google service works correctly for me now on port 80.

Regarding the the dangerous assumption, do you think it's OK to restore a config and then just try it? If so, I'm really close to having this done. I've been trying to learn how a separate python program could load the right classes so that it could call settings.load() to test the config data. If that's over the top it would make things a whole lot simpler. And, as I said before, what to do if the settings did not load properly?

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

No branches or pull requests

3 participants