search

sugarlabs / sugar

Sugar GTK shell
GNU General Public License v3.0
252 stars 240 forks source link

Major Fixes #922

Closed Saumya-Mishra9129 closed 3 years ago

Saumya-Mishra9129 commented 4 years ago

Fixes #855 . @quozl @srevinsaju Please Review.

chimosky commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@chimosky @srevinsaju Please Review.

chimosky commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Given my research as to the cause of https://github.com/sugarlabs/sugar/issues/923 and looking only at https://github.com/sugarlabs/sugar/pull/922/commits/efb71f6be3ff98593192aa2c1551373f7df087c4 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 commented 4 years ago

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 commented 4 years ago

I'll wait for you to change the commit and message on https://github.com/sugarlabs/sugar/commit/efb71f6be3ff98593192aa2c1551373f7df087c4. It seems clear to me that aa18879 used decode instead of encode and wasn't tested.

Saumya-Mishra9129 commented 4 years ago

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 commented 4 years ago

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

quozl commented 4 years ago

Thanks. Reviewed.

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

Saumya-Mishra9129 commented 3 years ago

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.

Saumya-Mishra9129 commented 3 years ago

@quozl I have made requested changes , Please review.

quozl commented 3 years ago

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

One of the patches was not cherry-picked;

Saumya-Mishra9129 commented 3 years ago

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 https://github.com/sugarlabs/sugar/blob/118fe73eee4177833dc692e1172451656f580b7f/src/jarabe/journal/journalentrybundle.py#L62 reads the preview, and passes the output returned by _read_preview to dbus.ByteArray in https://github.com/sugarlabs/sugar/blob/118fe73eee4177833dc692e1172451656f580b7f/src/jarabe/journal/journalentrybundle.py#L64 , dbus.ByteArray takes bytes data as argument , but in _read_preview https://github.com/sugarlabs/sugar/blob/118fe73eee4177833dc692e1172451656f580b7f/src/jarabe/journal/journalentrybundle.py#L90 file is opened as 'r' only.

@quozl Ping

quozl commented 3 years ago

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