Major Fixes #922

Saumya-Mishra9129 · 2020-06-03T19:27:29Z

Fixes #855 . @quozl @srevinsaju Please Review.

srevinsaju

Perfect 👍

chimosky · 2020-06-05T23:03:07Z

Haven't reviewed 491305e yet but there was a mention of a similar traceback in the mailing list, they're 5 definitions of get_bundle_id() in sugar, can you track and make sure they're all strings and not bytes so they wouldn't be decoded. Thanks.

Saumya-Mishra9129 · 2020-06-06T06:49:15Z

Haven't reviewed 491305e yet but there was a mention of a similar traceback in the mailing list, they're 5 definitions of get_bundle_id() in sugar, can you track and make sure they're all strings and not bytes so they wouldn't be decoded. Thanks.

Thanks for suggestion , I am sure Its going to help me in further fixes.

Saumya-Mishra9129 · 2020-06-08T18:27:12Z

@chimosky @srevinsaju Please Review.

srevinsaju

Looks good. I am not good at this collaboration, sockets and channels , but by code review, its a safer methodology you have implemented. So good one! 👍

extensions/cpsection/modemconfiguration/model.py

chimosky · 2020-06-11T00:41:14Z

Reviewed, thanks.

Your commit messages in 7ef6d2e...1e4709a is misleading as you use unicode and string interchangeably and they both meant different things in python2.

As an instance, in 7ef6d2e you said;
" Use byte instead of unicode
ByteArray objects must be initialized with bytes objects, not unicodes."

but bytes is used instead of a string and not unicode.

Saumya-Mishra9129 · 2020-06-11T07:37:01Z

Reviewed, thanks.

Your commit messages in 7ef6d2e...1e4709a is misleading as you use unicode and string interchangeably and they both meant different things in python2.

As an instance, in 7ef6d2e you said;
" Use byte instead of unicode
ByteArray objects must be initialized with bytes objects, not unicodes."

but bytes is used instead of a string and not unicode.

@chimosky I have done as suggested by you.

quozl · 2020-06-15T03:01:12Z

Given my research as to the cause of #923 and looking only at efb71f6 it doesn't look like you tested this before writing it (get_bundle_id should always returns a str so there's no need to check for type), so it doesn't look like the right solution. How did you test? Was there ever an instance where something other than a str was returned, and if so how did that happen?

Saumya-Mishra9129 · 2020-06-15T06:26:35Z

Given my research as to the cause of #923 and looking only at efb71f6 it doesn't look like you tested this before writing it (get_bundle_id should always returns a str so there's no need to check for type), so it doesn't look like the right solution. How did you test? Was there ever an instance where something other than a str was returned, and if so how did that happen?

Thanks for reviewing. get_bundle_id always returns a str. I agree with you. The way I have done it is wrong. However md5 hash function accepts sequence of bytes. So rest part is correct.

quozl · 2020-06-16T05:19:56Z

I'll wait for you to change the commit and message on efb71f6. It seems clear to me that aa18879 used decode instead of encode and wasn't tested.

Saumya-Mishra9129 · 2020-06-16T17:48:12Z

I'll wait for you to change the commit and message on efb71f6. It seems clear to me that aa18879 used decode instead of encode and wasn't tested.

It is done in d97fff2. @quozl You can review now.

Saumya-Mishra9129 · 2020-07-06T15:04:01Z

I think this Pull request can merge now. @quozl @chimosky @JuiP @srevinsaju Please review and suggest.

srevinsaju

Reviewed. Looks good

quozl · 2020-07-07T05:50:08Z

Thanks. Reviewed.

in 4de8eaf the warning does not mention size, but the code that uses size is changed; is this change needed? Also, it isn't clear why this has begun to be a problem now. Also, there are many (30) other calls to set_markup that are not being protected by markup_escape_text; were these considered? For instance if a journal entry is named &. The commit message should explain.
in 854b798 it isn't clear what problem is being solved. The commit message should explain.
in 04de77f under what circumstances will ConfigParser return anything that is not str, and if never why not apply the encode directly to the config.get? The commit message should explain.

The commit messages are not in style for the Sugar repository;

"Use GLib.markup_escape_text() before set_markup " could be "Fix Gtk-WARNING in language control panel",
"Fix _read_preview : read in binary mode" would be better explained another way, but I don't yet know what the problem is that is being solved,
"Use bytes as argument instead of string", "Use byte in ssid instead of string", "String argument without an encoding" and "invalid comparison - can raise to errors" could be "Use bytes for SSIDs",
" 'str' object has no attribute 'decode'" should be "Fix AttributeError in SCENARIO'" where SCENARIO explains what is done to cause it, e.g. opening modem control panel.

Saumya-Mishra9129 · 2020-08-15T20:12:59Z

in 4de8eaf the warning does not mention size, but the code that uses size is changed; is this change needed? Also, it isn't clear why this has begun to be a problem now. Also, there are many (30) other calls to set_markup that are not being protected by markup_escape_text; were these considered? For instance if a journal entry is named &. The commit message should explain.

@quozl I agree , if there are 30 calls to set_markup, we should protect them by markup_esacpe_text if there is a probability of getting '&', '>' or '<' sign.

in 04de77f under what circumstances will ConfigParser return anything that is not str, and if never why not apply the encode directly to the config.get? The commit message should explain.

Thanks Yeah configparser will return str, You are correct.

Use GLib.markup_escape_text() before set_markup. Sometimes there can be characters in label such that markup parser can fail to parse the markup. markup_escape_text escapes text so that the markup parser will parse it. Less than, greater than, ampersand, etc. will be replaced with the corresponding entities. Fixes Warning - main.py:6165): Gtk-WARNING **: 17:56:49.381: Failed to set text '<span foreground=#000000>Antigua & Barbuda</span>' from markup due to error parsing markup: Error on line 1: Entity did not end with a semicolon; most likely you used an ampersand character without intending to start an entity — escape ampersand as &

preview_path is a binary file. Binary file was being read as text. Regression introduced by aa18879 ("Port to Python 3"). Similar change in sugarlabs@babf1a5

ByteArray objects must be initialized with bytes objects, not strings. It raises error - "string argument without an encoding" Regression made in sugarlabs@aa18879

ssid is of type <class bytes> A regression in sugarlabs@aa18879#diff-648be2a40679960f79033120a92b1daaR704 We cannot decode a <class str> object. decode() works with <class bytes> only i.e. encoded object

ssid is of type <class bytes> in Python3, comparison of it with <class str> object results False.

ssid is of type <class bytes> in Python3, comparison of it with <class str> object results false.

https://github.com/sugarlabs/sugar/blob/d59b62a5f1db8c1e1ecb7ccbbcc8b85163290908/extensions/cpsection/modemconfiguration/model.py#L318 already returns a 'str' object, there is no need to decode from 'UTF-8'. Regression in aa18879#diff-faa2d318ce1ef21991d9ca1c6fa958ac Fixes following Erros: Traceback (most recent call last): File "/usr/lib/python3/dist-packages/jarabe/controlpanel/gui.py", line 459, in __select_option_cb self.show_section_view(option) File "/usr/lib/python3/dist-packages/jarabe/controlpanel/gui.py", line 313, in show_section_view self._section_view = view_class(model, File "/usr/share/sugar/extensions/cpsection/modemconfiguration/view.py", line 125, in __init__ self.service_providers = self._model.ServiceProviders() File "/usr/share/sugar/extensions/cpsection/modemconfiguration/model.py", line 279, in __init__ country_code, provider_name, plan_idx = self._get_initial_config() File "/usr/share/sugar/extensions/cpsection/modemconfiguration/model.py", line 332, in _get_initial_config provider_name = provider_name.decode('utf-8') AttributeError: 'str' object has no attribute 'decode'

t is reproduced by choosing Favorites View - Random View from the toolbar in the Home View. Impact is that the favourites view disappears and is empty. Fixes sugarlabs#923 Regression made in aa18879#diff-685d653cc9490267e474dabc5c40b108R249 get_bundle_id always returns a str, encode it md5 hash function accepts sequence of bytes Reported by - Shaan Subbaiah <shaansubbaiah.cs18@bmsce.ac.in> Traceback (most recent call last): File /usr/lib/python3/dist-packages/jarabe/desktop/viewcontainer.py, line 69, in do_size_allocate self._layout.allocate_children(allocation, self._children) File /usr/lib/python3/dist-packages/jarabe/desktop/favoriteslayout.py, line 250, in allocate_children name_hash = hashlib.md5(child.get_bundle_id().decode()) AttributeError: 'str' object has no attribute 'decode'

Use bytes for ssid Dbus.ByteArray takes encoded bytes i.e. <class bytes> as an argument in Python3. Documentation<https://dbus.freedesktop.org/doc/dbus-python/PY3PORT.html> says ' ByteArray objects must be initialized with bytes objects, not unicodes. Use b’’ literals in the constructor.' ConfigParser's get method returns <str> object. Encode str to bytes before using it in ByteArrays. Regression introduced in sugarlabs@aa18879 ("Port to Python 3")

Saumya-Mishra9129 · 2020-08-16T20:27:43Z

@quozl I have made requested changes , Please review.

quozl · 2020-08-18T02:29:35Z

Thanks. I've cherry-picked and merged some of the patches. Please rebase.

3f1fb18 was picked, the change to line 472 was removed (as I had pointed out the warning did not include size), and pushed as ea2cd9c,
54dfb70, df715e3, f370dfe, 785f42e and 7024fc6 were picked, collapsed, commit message rewritten, and pushed as 9900ccf,
a211893 was picked, commit message rewritten, and pushed as e0496b0,
137584d was picked, commit message rewritten, and pushed as 118fe73,

One of the patches was not cherry-picked;

637bbd8 wasn't clear to me from the commit message how to reproduce the problem, or what the problem is,

Saumya-Mishra9129 · 2020-08-23T20:08:59Z

637bbd8 wasn't clear to me from the commit message how to reproduce the problem, or what the problem is,

Thanks , the intuition behind this is that

sugar/src/jarabe/journal/journalentrybundle.py

Line 62 in 118fe73

preview = self._read_preview(temp_uid, bundle_dir)

reads the preview, and passes the output returned by _read_preview to dbus.ByteArray in

sugar/src/jarabe/journal/journalentrybundle.py

Line 64 in 118fe73

metadata['preview'] = dbus.ByteArray(preview)

,
dbus.ByteArray takes bytes data as argument , but in _read_preview

sugar/src/jarabe/journal/journalentrybundle.py

Line 90 in 118fe73

f = open(preview_path, 'r')

file is opened as 'r' only.

@quozl Ping

quozl · 2020-09-15T00:08:38Z

Sorry for the delay, notification was filed as spam. Thanks, merged as 05b4ca0.

Saumya-Mishra9129 force-pushed the markup-text branch from 38a444a to 30153e0 Compare June 3, 2020 19:31

srevinsaju approved these changes Jun 4, 2020

View reviewed changes

Saumya-Mishra9129 changed the title ~~Use GLib.markup_escape_text() before set_markup~~ Major Fixes Jun 5, 2020

Saumya-Mishra9129 force-pushed the markup-text branch from 2a0c30d to 491305e Compare June 5, 2020 10:15

srevinsaju mentioned this pull request Jun 6, 2020

Sugar 117-3 on Debian - AttributeError: 'str' object has no attribute 'decode' #923

Closed

srevinsaju suggested changes Jun 8, 2020

View reviewed changes

extensions/cpsection/modemconfiguration/model.py Outdated Show resolved Hide resolved

Saumya-Mishra9129 force-pushed the markup-text branch 2 times, most recently from cfec35e to 9b05557 Compare June 9, 2020 06:22

Saumya-Mishra9129 force-pushed the markup-text branch 3 times, most recently from b4bcdce to 02ddae9 Compare June 11, 2020 07:35

Saumya-Mishra9129 force-pushed the markup-text branch from 02ddae9 to d97fff2 Compare June 16, 2020 17:46

Saumya-Mishra9129 force-pushed the markup-text branch from d97fff2 to 0c0cea4 Compare June 20, 2020 10:49

srevinsaju approved these changes Jul 6, 2020

View reviewed changes

Saumya-Mishra9129 added 4 commits August 16, 2020 01:52

Fix filesystem preview read

637bbd8

preview_path is a binary file. Binary file was being read as text. Regression introduced by aa18879 ("Port to Python 3"). Similar change in sugarlabs@babf1a5

Use bytes for SSIDs

54dfb70

ByteArray objects must be initialized with bytes objects, not strings. It raises error - "string argument without an encoding" Regression made in sugarlabs@aa18879

Use bytes for SSIDs

df715e3

ssid is of type <class bytes> A regression in sugarlabs@aa18879#diff-648be2a40679960f79033120a92b1daaR704 We cannot decode a <class str> object. decode() works with <class bytes> only i.e. encoded object

Saumya-Mishra9129 force-pushed the markup-text branch from 0c0cea4 to 50d0c93 Compare August 15, 2020 20:33

Saumya-Mishra9129 added 5 commits August 17, 2020 01:51

Use bytes for SSIDs

f370dfe

ssid is of type <class bytes> in Python3, comparison of it with <class str> object results False.

Use bytes for SSIDs

785f42e

ssid is of type <class bytes> in Python3, comparison of it with <class str> object results false.

Saumya-Mishra9129 force-pushed the markup-text branch from 50d0c93 to 7024fc6 Compare August 16, 2020 20:26

quozl closed this Sep 15, 2020

Major Fixes #922

Major Fixes #922

Saumya-Mishra9129 commented Jun 3, 2020 •

edited

srevinsaju left a comment •

edited

chimosky commented Jun 5, 2020

Saumya-Mishra9129 commented Jun 6, 2020

Saumya-Mishra9129 commented Jun 8, 2020

srevinsaju left a comment

chimosky commented Jun 11, 2020

Saumya-Mishra9129 commented Jun 11, 2020

quozl commented Jun 15, 2020 •

edited

Saumya-Mishra9129 commented Jun 15, 2020

quozl commented Jun 16, 2020

Saumya-Mishra9129 commented Jun 16, 2020 •

edited

Saumya-Mishra9129 commented Jul 6, 2020

srevinsaju left a comment

quozl commented Jul 7, 2020

Saumya-Mishra9129 commented Aug 15, 2020 •

edited

Saumya-Mishra9129 commented Aug 16, 2020

quozl commented Aug 18, 2020 •

edited

Saumya-Mishra9129 commented Aug 23, 2020 •

edited

quozl commented Sep 15, 2020

Major Fixes #922

Major Fixes #922

Conversation

Saumya-Mishra9129 commented Jun 3, 2020 • edited

srevinsaju left a comment • edited

Choose a reason for hiding this comment

chimosky commented Jun 5, 2020

Saumya-Mishra9129 commented Jun 6, 2020

Saumya-Mishra9129 commented Jun 8, 2020

srevinsaju left a comment

Choose a reason for hiding this comment

chimosky commented Jun 11, 2020

Saumya-Mishra9129 commented Jun 11, 2020

quozl commented Jun 15, 2020 • edited

Saumya-Mishra9129 commented Jun 15, 2020

quozl commented Jun 16, 2020

Saumya-Mishra9129 commented Jun 16, 2020 • edited

Saumya-Mishra9129 commented Jul 6, 2020

srevinsaju left a comment

Choose a reason for hiding this comment

quozl commented Jul 7, 2020

Saumya-Mishra9129 commented Aug 15, 2020 • edited

Saumya-Mishra9129 commented Aug 16, 2020

quozl commented Aug 18, 2020 • edited

Saumya-Mishra9129 commented Aug 23, 2020 • edited

quozl commented Sep 15, 2020

Saumya-Mishra9129 commented Jun 3, 2020 •

edited

srevinsaju left a comment •

edited

quozl commented Jun 15, 2020 •

edited

Saumya-Mishra9129 commented Jun 16, 2020 •

edited

Saumya-Mishra9129 commented Aug 15, 2020 •

edited

quozl commented Aug 18, 2020 •

edited

Saumya-Mishra9129 commented Aug 23, 2020 •

edited