offline-qr-code
offline-qr-code copied to clipboard
rugk
Radio options not correctly reset when reset button is pressed
STR:
- Go to options.
- Change some radio options (qr code size or type)
- Click "reset".
What happens: Old setting is still displayed.
What should happen: Reset this option, too.
(discovered in https://github.com/rugk/offline-qr-code/commit/918e423b25dec4624acda2393579b673e3324632, may be there in other versions, too)
I will work on this, forked the repo, however, I am contributing for the first time.
Okay, note that this may not be an easy issue (I have not looked into it, but some debugging skills may be required). If you have any questions feel free to ask.
So only QR code type and size properties are not reset. High level debugging revealed that QR code type has not handling in apply option. And the size has to be handled for all conditions.
Need a bit more understanding on the approach for resetting. I see there is a constant object for default values but applyOptionLive method utilize that in case of reset.
Need a bit more understanding on the approach for resetting.
Speaking on a high level the resetting works like this:
- backup old settings for restoring them
- deletes all settings from storage
- fetches new default settings (indirectly from that object you've mentioned AFAIK) and applies them as if they had been re-loaded
applyOptionLive does not directly relate to resetting, but could be a culprit/mess some things up, indeed. As the JSdoc hopefully explains (:wink:) it just applies some special features of custom options (such as disabling the input field of the qr code size when the option is not selected).
You can also try to look which commit introduced that behaviour, but I am not sure whether this is helpful…
@rugk I do have an idea about code changes to be done. But I have never worked on extensions so I looked in contributing.md and tried editing src in add-on debugger (which doesn't allow edit). Can you please guide me with that?
Actually the Contributing.md has a small part for getting starting. I guess you checked that out already. So for testing/changing things, you actually need to modify the source code in any text/file editor/IDE outside of the browser and reload the add-on.
I have installed the addon from the store. I haven’t generated it using make.sh So I suppose I need to build and install that addon?
You don't need to run any build script. You can load the add-on instantly, you just have to clone the repo here and you can load the manifest.json directly. How to do that is explained here.
@OogieBoogieInJSON I was able to get to the cause however kinda packed up with other work. I will continue to work on it this weekend!
If you can describe it in a compact way, it would certainly also not hurt to describe the cause in this issue, even if you have not a PR ready yet. :wink: (So others could confirm/pick it up, even if you do implement it or so, but well… if you fix it anyway) So srsly, it does not matter if you really just get to it this weekend. We are not in hurry… :wink:
@OogieBoogieInJSON Take a close look at ApplyLive() method. It is being called while setting custom options & resetting to default options. It is missing a handler for 'qrcodesize' and 'qrcodetype'. Therefore, other options reset to default just fine but these two don't reflect the change on reset.
Might be the culprit, but actually the applyOptionLive() method should only apply custom stuff of options, i.e. not every option needs to be defined there just to be saved/reset. I've explained that high-level stuff above, already.
FYI I've seen the JSDOC for this method is bad, I'll improve it.
@TauseefMalik BTW, can you still reproduce this with the latest version? Note we now have proper unit tests for all used (TinyWebExt) modules and they are also properly implemented.
I suspect https://github.com/rugk/offline-qr-code/issues/180 could fix this issue. Needs verification though… :smiley:
Result: Still fails. Also when updating AddonSettings and AutomaticSettings modules, the initial reset now works, but when I press "Undo", it does not correctly reset the options to their previous values…