Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

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

Open
peterflynn opened this issue Jul 22, 2013 · 10 comments
Open

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

peterflynn opened this issue Jul 22, 2013 · 10 comments
Labels
F Extension Manager low priority

Comments

@peterflynn
Copy link
Member

  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.
@peterflynn
Copy link
Member Author

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

@ghost ghost assigned njx Jul 22, 2013
@njx
Copy link
Contributor

njx commented Jul 22, 2013

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

njx added a commit that referenced this issue Jul 22, 2013
@njx
For #4535, show error message but not 'remove' link for bad extension…
da1ec18 
… in dev folder
@njx njx mentioned this issue Jul 22, 2013
Merged
@njx
Copy link
Contributor

njx commented Jul 22, 2013

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

jasonsanjose added a commit that referenced this issue Jul 22, 2013
@jasonsanjose
Merge pull request #4538 from adobe/nj/issue-4535
31fdaba 
For #4535, show error message but not 'remove' link for bad extension in dev folder
@ghost ghost assigned peterflynn Jul 22, 2013
@jasonsanjose
Copy link
Member

FBNC @peterflynn

@peterflynn
Copy link
Member Author

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.

@ghost ghost assigned njx Jul 24, 2013
@njx
Copy link
Contributor

njx commented Jul 25, 2013

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?

@peterflynn
Copy link
Member Author

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

@larz0
Copy link
Member

larz0 commented Jul 25, 2013

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

screen shot 2013-07-25 at 11 19 36 am

@peterflynn
Copy link
Member Author

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

@larz0
Copy link
Member

larz0 commented Jul 26, 2013

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.

peterflynn added a commit that referenced this issue Jul 30, 2013
@peterflynn
Merge remote-tracking branch 'origin/master' into pflynn/unit-test-cl…
8d94d0d 
…eanups

* origin/master: (30 commits)
  turn off optimization in acorn (but not acorn_loose right now)
  cleanup unit test prefs
  temporarily switch to my tern
  Revert "Revert "Workaround for the Tern crash.""
  Update README.md
  Work around #4554 (Extension Manager font is hard to read on Windows), which is a Chromium bug, by avoiding the lightest font-weight on Win. Lighten the text slightly so it's still a little muted, like the design looks on Mac and with older CEFs on Win.
  Revert "Workaround for the Tern crash."
  Updated by ALF automation.
  Re-add toolbar hover. Fix some button appearance issues.
  Updated by ALF automation.
  * Fix bug #4548 - remove Save As from folder tree context menu * Fix exception thrown when File > Save As invoked with nothing open * Update docs for working set events to reflect PR #4450
  integration tests for registerInlineEditProvider
  Fixes after review
  JSDoc fixes.
  Fix for extensions compare
  show error message and add safety check
  For #4535, show error message but not 'remove' link for bad extension in dev folder
  Refactor provider callback for export
  Generalize registerInlineEditProvider and registerInlineDocProvider to take an optional priority parameter
  change upper limit to 16000
  ...
@njx njx removed their assignment Jul 12, 2014
@core-ai-bot core-ai-bot mentioned this issue Aug 31, 2021
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
F Extension Manager low priority
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@njx @jasonsanjose @peterflynn @larz0