Skip to content

Conversation

@quozl
Copy link

@quozl quozl commented May 25, 2020

Hard coded integer constants were used instead of enums imported from
TelepathyGLib.

Reported-by: Saumya Mishra [email protected]

Hard coded integer constants were used instead of enums imported from
TelepathyGLib.

Reported-by: Saumya Mishra <[email protected]>
Copy link
Member

@Saumya-Mishra9129 Saumya-Mishra9129 left a comment

Choose a reason for hiding this comment

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

Reviewed, Looks good to me

@Saumya-Mishra9129
Copy link
Member

Gentle reminder, @quozl you can merge it .

This reverts commit b740323.

Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/dbus/connection.py", line 230, in maybe_handle_message
    self._handler(*args, **kwargs)
  File "/usr/share/sugar/activities/Pippy.activity/collabwrapper.py", line 845, in _received_cb
    self._activity_cb(buddy, msg)
  File "/usr/share/sugar/activities/Pippy.activity/collabwrapper.py", line 315, in __received_cb
    ACTIVITY_FT_MIME)
  File "/usr/share/sugar/activities/Pippy.activity/collabwrapper.py", line 745, in __init__
    self._blob = blob.encode('utf-8')
AttributeError: 'bytes' object has no attribute 'encode'

Signed-off-by: James Cameron <[email protected]>
@quozl
Copy link
Author

quozl commented May 28, 2020

It didn't work. Turned out to be something else. Related sugarlabs/Pippy#82. Needs wider testing.

@Saumya-Mishra9129
Copy link
Member

Saumya-Mishra9129 commented Jun 7, 2020

It didn't work. Turned out to be something else. Related sugarlabs/Pippy#82. Needs wider testing.

Yeah I agree here. 33ebbd6 , it seems correct to me. But its not working so Possibly there is another regression in Pippy. I still need to test it.
Because @srevinsaju tested it with physics earlier in sugarlabs/physics#44 , but we haven't tested with all other activities.
Edit - I just got the real cause , I compared collabwrapper of Pippy with actual one , find out sugarlabs/Pippy@b655240 is extra and causing the issue here in Pippy. We need to fix this one.
@chimosky what do you say?

@quozl
Copy link
Author

quozl commented Jun 16, 2020

We also have to make sure to fix problems in the layer they occur in, rather than exactly where the traceback points.

API documentation for CollabWrapper, with types for objects, may help us to concentrate the change on the correct layer. Several porting problems may have been fixed by either editing activity or CollabWrapper sources.

@Saumya-Mishra9129
Copy link
Member

Saumya-Mishra9129 commented Jun 16, 2020

Several porting problems may have been fixed by either editing activity or CollabWrapper sources.

But the main point is that, we maintain collabwrapper as upstream source. So we should not allow changes there at the time of porting in the activity. The collabwarapper files in main upstream and pippy differs which can cause problems while testing (if someone new goes to pippy and test collabwrapper changes there) .

Correct me If I am wrong.

@quozl
Copy link
Author

quozl commented Jun 16, 2020

I'm not going to disallow changes to CollabWrapper in activity source. Sometimes a change there may be the only way to solve an immediate problem in the time available. The change ought to be considered for upstreaming to CollabWrapper, and when that happens the change must undergo review, should not change the API without justification, and the CollabWrapper test activity should be able to test the change and the test must pass.

@Saumya-Mishra9129
Copy link
Member

I'm not going to disallow changes to CollabWrapper in activity source. Sometimes a change there may be the only way to solve an immediate problem in the time available. The change ought to be considered for upstreaming to CollabWrapper, and when that happens the change must undergo review, should not change the API without justification, and the CollabWrapper test activity should be able to test the change and the test must pass.

Fine thanks. I think a better way would be to test these changes with fructose activities.

@quozl
Copy link
Author

quozl commented Jun 19, 2020

That would happen when a fructose activity adopts changes from CollabWrapper. What is important for CollabWrapper is that the test activity demonstrates and tests the API.

@Saumya-Mishra9129
Copy link
Member

Saumya-Mishra9129 commented Jun 22, 2020

@quozl why you are reverting the change in 33ebbd6. I tried with test activity - master, it works fine, no such error in log. No need to revert the change here as in 33ebbd6, but yeah it will not work in Pippy because of different issues in Pippy. Getting same error after reverting with test activity,

Traceback (most recent call last):
  File "/usr/share/sugar/activities/collabwrapper/test/collabwrapper.py", line 696, in __notify_state_cb
    input_stream = self._get_input_stream()
  File "/usr/share/sugar/activities/collabwrapper/test/collabwrapper.py", line 744, in _get_input_stream
    return Gio.MemoryInputStream.new_from_data(self._blob, None)
TypeError: Item 0: Must be number, not str

However , while testing with master I found that the data recieved is bytes data i.e. we send data ('One Two Three', 'Test Data') with host and we get b'One Two Three' on guest machine. Shouldn't it be str instead of byte.??

@Saumya-Mishra9129
Copy link
Member

I don't think 33ebbd6 is needed.

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