Skip to content
This repository has been archived by the owner. It is now read-only.

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

Merged
merged 6 commits into from Mar 23, 2017

Conversation

@nihakhanna
Copy link
Collaborator

@nihakhanna nihakhanna commented Mar 19, 2017

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
Copy link
Contributor

@dannycoates dannycoates commented Mar 20, 2017

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
Copy link
Collaborator Author

@nihakhanna nihakhanna commented Mar 20, 2017

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
Copy link
Contributor

@dannycoates dannycoates commented Mar 20, 2017

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
Copy link
Collaborator Author

@nihakhanna nihakhanna commented Mar 21, 2017

Makes complete sense. On it! :) @dannycoates

@nihakhanna
Copy link
Collaborator Author

@nihakhanna nihakhanna commented Mar 21, 2017

@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 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
@nihakhanna
Copy link
Collaborator Author

@nihakhanna nihakhanna commented Mar 22, 2017

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));
Copy link
Contributor

@dannycoates dannycoates Mar 22, 2017

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:

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.

Copy link
Collaborator Author

@nihakhanna nihakhanna Mar 23, 2017

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!

Copy link
Contributor

@dannycoates dannycoates left a comment

Excellent work @niharikak101!

@dannycoates dannycoates merged commit 05c2a00 into mozilla-services:master Mar 23, 2017
@nihakhanna
Copy link
Collaborator Author

@nihakhanna nihakhanna commented Mar 23, 2017

@dannycoates aw, thanks to you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants