llaske / sugarizer

Sugarizer is a web implementation of the Sugar platform to run on any device or browser
https://sugarizer.org
Apache License 2.0
197 stars 411 forks source link

Duplicated text in QR Code Activity history #314

Closed yog24esh closed 5 years ago

yog24esh commented 5 years ago

I think there is no use of logging the same user in the dropdown menu if he generate his qrcode again.

Here is the ex-

Screenshot (38)

Here what to be expected -

Screenshot (39)

Scar26 commented 5 years ago

working on this

yog24esh commented 5 years ago

Have a look in qrtextdropdown id and attach the if statement to check if previous div value is same or not.

Screenshot (42)

Scar26 commented 5 years ago

This wouldn't work. the names are being stored in a global array called history. If I run the condition check at the div change,the array would still be different every time because it would just contain the same name multiple times. Instead I attached the statement at the beginning of the pushtohistory function so if the name already exists in the array it's not pushed at all

yog24esh commented 5 years ago

Yes but instead of checking the whole array , checking the last pushed element in the array would be useful otherwise it would defeat the purpose of history. Ex - If a user1 generate qrcode. arr->[user1] user2 generate qrcode. arr->[user1,user2] user1 again generate qrcode. arr->[user1,user2,user1] user1 is already present in array but it should be logged because it shows the history.

Scar26 commented 5 years ago

Hm...makes sense,I mean I probably could just let it be logged into history and run the test in the for loop that they are using to append in the div. But it would be a little less efficient since we have to check a higher number of elements every-time. On the other hand, it makes sense that history should contain everything.

@llaske can we have your opinion on this?

llaske commented 5 years ago

Easy access to old generated QR code is more useful than a real history with duplication.

So https://github.com/llaske/sugarizer/pull/317 fixed the issue in my opinion. A supplemental fix is here: https://github.com/llaske/sugarizer/pull/339

llaske commented 5 years ago

Closed in https://github.com/llaske/sugarizer/commit/dd3c58d56981e4dfa6d94601b95d7117a3bc9241