fix: Link copy popup is not displayed when request fails (WIP) #2422
Conversation
Nice catch @niharikak101 :) This approach does ensure the result won't be true if
There's not really a right or wrong answer. We can do whatever we think is best as long as we make it clear. What do you think it should do? |
Hey @dannycoates thank you for breaking it down so nicely! :) |
Cool, so we're saying that we always expect
Right, and this helps us decide what to do for the larger problem. Now we're stepping further back from There's lots of things we could do, ranging from a simple error message to a complicated feature that stores the screenshot locally and sends it to the server when it becomes available again. We need to consider how much time we can afford to spend on it. It's often a good decision to do a quick thing first to solve the immediate need then make it better if its a common problem. I'll tell you right now we're pretty busy so a quick fix is definitely the way to go. :) I think an error message is a good thing to do in this case. It tells the user something unexpected happened and that it didn't work. Then they can choose what to do: try again, forget about it, etc. I don't think we should open a tab to anywhere. I would probably be confused if that happened to me, like "Where did my screenshot go? I don't see it". I encourage you to work through that scenario and write some code to show an informative error message. Let me know if you have questions about the code or anything. |
Makes complete sense. On it! :) @dannycoates |
@dannycoates |
I added some changes to close the /creating tab (#2446) if the upload fails. |
.then((tabs) => { | ||
for (let tab of tabs) { | ||
if(tab.url.match(/\/creating/i)) { | ||
catcher.watchPromise(browser.tabs.remove(tab.id)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@niharikak101 this is good but I think there's a way we can do this without looking through all the tabs.
Look at this section of code:
screenshots/addon/webextension/background/takeshot.js
Lines 41 to 51 in dd75b42
return catcher.watchPromise(capturePromise.then(() => { | |
return browser.tabs.create({url: shot.creatingUrl}) | |
}).then((tab) => { | |
openedTab = tab; | |
return uploadShot(shot); | |
}).then(() => { | |
return browser.tabs.update(openedTab.id, {url: shot.viewUrl}); | |
}).then(() => { | |
return shot.viewUrl; | |
})); | |
})); |
We can add another function to the then
after uploadShot
to catch any errors and remove the tab. Handling the error close to where it happens is usually a good practice. In this case we still have a reference to the tab we opened, openedTab
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review @dannycoates !
I tried adding a second function parameter to then
, but wasn't very sure about how to break out of the Promise chain because the promise was still getting resolved (possibly because the tabs.remove
returns a promise that is resolved if the tab is closed). It was still showing a copied to clipboard notification. I added a catch
to the chain, and it seems to solve the problem. Let me know what you think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work @niharikak101!
@dannycoates aw, thanks to you! |
Fixes #2416
Seems like the document.execCommand method returns true even when the url is undefined. (According to the docs: "It returns a Boolean that is false if the command (in this case, 'copy') is not supported or enabled"), just added a check to see whether 'text' is undefined or not.
Not sure if this is the right approach, thoughts?