search

sugarlabs / sugar-toolkit-gtk3

Sugar Learning Environment, Activity Toolkit, GTK 3.
GNU Lesser General Public License v2.1
21 stars 80 forks source link

Minor Fixes #451

Closed Saumya-Mishra9129 closed 4 years ago

Saumya-Mishra9129 commented 4 years ago

@chimosky @quozl Please review.

quozl commented 4 years ago

Thanks. Reviewed.

Saumya-Mishra9129 commented 4 years ago

3a7cc36 seems unlikely to be a correct solution to the error; best would be to verify from Python 3 source whether serve_forever properly handles EINTR, and if so remove the override. Same can be said for the other override in the class.

BaseServer class provides an attribute named __is_shut_down. But I will try other way as you suggested.

898b73d commit message doesn't have an adequate explanation; I've no idea why you are making this change.

The reason is that zoom method of style class takes int as argument.

quozl commented 4 years ago

Does it properly handle EINTR?

Saumya-Mishra9129 commented 4 years ago

Does it properly handle EINTR?

Yes it does , the above patch handles EINTR .

quozl commented 4 years ago

Does it properly handle EINTR?

Yes it does , the above patch handles EINTR .

Sorry, my question too brief; and was in context of my earlier comment;

verify from Python 3 source whether serve_forever properly handles EINTR, and if so remove the override. Same can be said for the other override in the class.

I'll put my question another way; what evidence do you have that EINTR is not handled by the Python 3 implementation of HTTPServer?

f52b4e1a96e34bacfd79e0f47099c295d9050f7d is where the code was first added, and the commit message says this is only used when the webkit1 API is enabled by explicitly setting an environment variable. bin/sugar-activity-web is the only reference to the source file.

But wait, webkit1 API is obsolete and unmaintained. Surely nobody is using it with Python 3?

from gi.repository import WebKit

So I'll ask another important question; why and how are you using the webkit1 API?

What led you to respond with https://github.com/sugarlabs/sugar-toolkit-gtk3/pull/451/commits/3a7cc36ac7c7633e68576f3a1099b8aee8a5440c in the first place? The commit message doesn't say. It's almost like you're making things up as you go along, and not widening your understanding of the code; what calls it and why. :grin:

Saumya-Mishra9129 commented 4 years ago

I'll put my question another way; what evidence do you have that EINTR is not handled by the Python 3 implementation of HTTPServer?

I tried to look at the source , it is nowhere explicitly mentioned in the code that it does, and even couldn't properly get the code. Also if we use handle_request in place of serve_forever, it does handles EINTR.

I agree nobody uses webkit1 in Python3. I didn't test the change also, because I couldn't find a way to test.

What led you to respond with 3a7cc36 in the first place?

I came up with the change because while reviewing the old commits, and checking with source I found we don't use _BaseServeris_shut_down in HTTPServer, it is now changed to is_shut_down attribute, so if somebody will try to use Webkit1 in future , he can get AttributeError. But I agree as I have no other reason to justify, I will now remove this commit , as webkit1 is not in use.

quozl commented 4 years ago

Thanks. As it has no purpose, you should remove support for WebKit1 API rather than maintain it. Given that WebKit2 is maintained, and the WebKit project owners have said WebKit1 will not be maintained, I don't see a good reason to keep WebKit1 code in the master branch.

Saumya-Mishra9129 commented 4 years ago

Thanks. As it has no purpose, you should remove support for WebKit1 API rather than maintain it. Given that WebKit2 is maintained, and the WebKit project owners have said WebKit1 will not be maintained, I don't see a good reason to keep WebKit1 code in the master branch.

@quozl Done in e2c6e70

quozl commented 4 years ago

Thanks, but incomplete. Added https://github.com/sugarlabs/sugar/pull/930, please review.

quozl commented 4 years ago

Reviewed again.

Saumya-Mishra9129 commented 4 years ago

6b00fcc casts to int the args of zoom yet zoom itself casts to int anyway; why make the change you have made instead of change the docstring? i.e. there were several ways to solve this and you've chosen a way that I would not have, so justify your decision. There's nothing in the commit message that describes how alternatives were deselected.

I agree there are other ways as well to solve it. I just thought I should avoid changing documentation. Tried it again in 3ba0572.

quozl commented 4 years ago

Thanks. Merged by hand with minor changes.