fix: Link copy popup is not displayed when request fails (WIP) #2422

nihakhanna · 2017-03-19T15:20:49Z

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?

dannycoates · 2017-03-20T01:29:53Z

Nice catch @niharikak101 :)

This approach does ensure the result won't be true if text is undefined, which is an improvement, now lets also consider what we might want this function to when it is.

What work do we need this function to do (like copying) if text is undefined?
When should we check if text is valid?
- after execCommand, before calling this function, at the beginning or end of this function?
Is it an error or does it just return something?

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?

nihakhanna · 2017-03-20T22:00:17Z

Hey @dannycoates thank you for breaking it down so nicely! :)
Because the clipboard.copy function is just copying the text to the clipboard, error handling of the text should probably be done before the function is called.
In that case, one way to immediately address the issue could be to redirect to the 'my shots' page (or somewhere else, if that is more appropriate).
The text being undefined seems to be because of a failure in uploading the screen-shotted image (and I think canvas.toDataUrl returning undefined, if I am not mistaken).

dannycoates · 2017-03-20T23:12:29Z

error handling of the text should probably be done before the function is called

Cool, so we're saying that we always expect text to be a string (and probably a non-empty one?). Since javascript isn't a statically typed language we can't just say text: string, so we should check that text is valid in our function before we do anything with it.

The text being undefined seems to be because of a failure in uploading the screen-shotted image

Right, and this helps us decide what to do for the larger problem. Now we're stepping further back from clipboard.copy and looking at it from a user's perspective. Imagine you've just clicked the "Save visible" button and the server is down so it can't be saved. What would be helpful to happen?

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.

nihakhanna · 2017-03-21T00:58:59Z

Makes complete sense. On it! :) @dannycoates

nihakhanna · 2017-03-21T15:37:52Z

@dannycoates
Made some changes!
This is reflecting some old commits as well, I can push to a new branch and submit a new PR, if you think that would be better, sorry about this.

nihakhanna · 2017-03-22T16:12:11Z

I added some changes to close the /creating tab (#2446) if the upload fails.

dannycoates · 2017-03-22T22:29:49Z

addon/webextension/background/main.js

+      .then((tabs) => {
+        for (let tab of tabs) {
+          if(tab.url.match(/\/creating/i)) {
+            catcher.watchPromise(browser.tabs.remove(tab.id));


@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.

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!

dannycoates

Excellent work @niharikak101!

nihakhanna · 2017-03-23T18:27:25Z

@dannycoates aw, thanks to you!

fix: Link copy popup is not displayed when request fails (WIP)

304a8e7

nihakhanna added 2 commits March 21, 2017 15:08

Test

52a4844

shows error when image is not saved

fa86bc1

nihakhanna added 2 commits March 22, 2017 14:03

merge

098678f

closes /creating tab if upload fails

e812186

nihakhanna changed the title ~~fix: Link copy popup is not displayed when request fails~~ fix: Link copy popup is not displayed when request fails (WIP) Mar 22, 2017

dannycoates reviewed Mar 22, 2017

View reviewed changes

closes tab if upload fails

5e58b4b

dannycoates approved these changes Mar 23, 2017

View reviewed changes

dannycoates merged commit 05c2a00 into mozilla-services:master Mar 23, 2017

ianb mentioned this pull request Mar 28, 2017

When upload fails, /creating tab should be closed #2446

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Link copy popup is not displayed when request fails (WIP) #2422

fix: Link copy popup is not displayed when request fails (WIP) #2422

nihakhanna commented Mar 19, 2017

dannycoates commented Mar 20, 2017

nihakhanna commented Mar 20, 2017

dannycoates commented Mar 20, 2017 •

edited

nihakhanna commented Mar 21, 2017 •

edited

nihakhanna commented Mar 21, 2017

nihakhanna commented Mar 22, 2017

dannycoates Mar 22, 2017

nihakhanna Mar 23, 2017

dannycoates left a comment

nihakhanna commented Mar 23, 2017

	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;
	}));
	}));

fix: Link copy popup is not displayed when request fails (WIP) #2422

fix: Link copy popup is not displayed when request fails (WIP) #2422

Conversation

nihakhanna commented Mar 19, 2017

dannycoates commented Mar 20, 2017

nihakhanna commented Mar 20, 2017

dannycoates commented Mar 20, 2017 • edited

nihakhanna commented Mar 21, 2017 • edited

nihakhanna commented Mar 21, 2017

nihakhanna commented Mar 22, 2017

dannycoates Mar 22, 2017

Choose a reason for hiding this comment

nihakhanna Mar 23, 2017

Choose a reason for hiding this comment

dannycoates left a comment

Choose a reason for hiding this comment

nihakhanna commented Mar 23, 2017

dannycoates commented Mar 20, 2017 •

edited

nihakhanna commented Mar 21, 2017 •

edited