Remove button enabled for "dev" extension if it has error

[core-ai-bot]

Issue by peterflynn
Monday Jul 22, 2013 at 19:13 GMT
Originally opened as adobe/brackets#4535

---

1. Create a new folder in src/extensions/dev
2. Place in it a main.js with syntax errors, or that throws an exception on startup... or don't place a main.js put place something else like todo.txt
3. (Re)load Brackets and open Extension Manager
4. Find your new folder's listing
5. Click the Remove button and allow Brackets to quit

Result: Remove button is enabled, even though normally it's disabled for everything in the "dev" folder. Its files are permanently deleted with no recourse after step 5.

Expected: Remove button is disabled just like with functioning dev extensions.

[core-ai-bot]

Comment by peterflynn
Monday Jul 22, 2013 at 19:14 GMT

---

@njx. Not sure if I'd quite call this a medium, but maybe... it is potential data loss for extension devs.

[core-ai-bot]

Comment by njx
Monday Jul 22, 2013 at 19:37 GMT

---

Good catch, I agree. Medium priority to me for this sprint.

[core-ai-bot]

Comment by njx
Monday Jul 22, 2013 at 21:06 GMT

---

I wonder if we should have some indication in the listing of which extensions are local "dev" extensions.

[core-ai-bot]

Comment by jasonsanjose
Monday Jul 22, 2013 at 21:17 GMT

---

FBNC@peterflynn

[core-ai-bot]

Comment by peterflynn
Wednesday Jul 24, 2013 at 17:11 GMT

---

Reopening as low priority to@njx -- the current behavior is a little inconsistent. Non-broken dev extensions show a disabled Remove button (with explanatory tooltip), while broken extensions show no Remove button at all.

[core-ai-bot]

Comment by njx
Thursday Jul 25, 2013 at 17:59 GMT

---

Yeah, that's a good point. The problem is that in the extension error case, we use a link rather than a button, and the disabled attribute doesn't work on links (and it's not clear users would think to tooltip over it anyway). So we'd need some different affordance to show this info.

@larz0 - any thoughts?

[core-ai-bot]

Comment by peterflynn
Thursday Jul 25, 2013 at 18:11 GMT

---

Any reason it needs to use a different affordance from the regular remove case? It does exactly the same action...

[core-ai-bot]

Comment by larz0
Thursday Jul 25, 2013 at 18:20 GMT

---

Is this what I'm supposed to see? There's an Extension Error instead of the Remove button.

[core-ai-bot]

Comment by peterflynn
Friday Jul 26, 2013 at 01:28 GMT

---

@larz0 Originally, we showed the error message and an enabled Remove button (or link, I guess). NJ's fix removed the link for dev extensions that shouldn't be deletable. In contrast, dev extensions that don't have an error still show the Remove UI, but with a tooltip explaining that dev externsions must be deleted manually. So I was expecting to see something matching that -- error message next to disabled link or button, rather no link/button at all.

I'm not sure why we use a Remove button in some cases and a Remove link in others, though.

[core-ai-bot]

Comment by larz0
Friday Jul 26, 2013 at 17:38 GMT

---

Let's remove the Remove UI for dev extensions that don't have an error and say something like "Must be removed manually" in its place.