Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added spaces between extensions when copying the list of extensions #195

Merged
merged 3 commits into from
Aug 23, 2016

Conversation

UtiluMark
Copy link
Contributor

When the list of extensions is copied to the Clipboard and pasted on a
location where newlines are not allowed, the extensions get glued
together:
Firebug 3.0.0-alpha.14Utilu Nightly Tester Tools 3.7.1.1Web Developer
1.2.7
After this change there is a space between the extensions:
Firebug 3.0.0-alpha.14 Utilu Nightly Tester Tools 3.7.1.1 Web Developer
1.2.7
Part of #188

…ns to the Clipboard

When the list of extensions is copied to the Clipboard and pasted on a
location where newlines are not allowed, the extensions get glued
together:
Firebug 3.0.0-alpha.14Utilu Nightly Tester Tools 3.7.1.1Web Developer
1.2.7
After this change there is a space between the extensions:
Firebug 3.0.0-alpha.14 Utilu Nightly Tester Tools 3.7.1.1 Web Developer
1.2.7
@UtiluMark
Copy link
Contributor Author

@whimboo Please review and merge.

@whimboo
Copy link
Contributor

whimboo commented Oct 6, 2015

Do you actually have an example where that happens? If new lines are not allowed they should at least replaced by a space.

@UtiluMark
Copy link
Contributor Author

It happens even in Firefox itself, try pasting the list of extensions in for example the address bar or the search bar. The newlines are removed, not replaced by spaces. This also happens in for example File Explorer.

@UtiluMark
Copy link
Contributor Author

@whimboo Do you need more examples?

@tonymec
Copy link
Contributor

tonymec commented Oct 9, 2015

Maybe replace the newlines by commas instead? Or replace NL by comma+space? I see there are spaces in the names of most extensions, and spaces vs. spaces might be confusing.

@@ -361,7 +361,7 @@ getExtensionList: function(callback) {
+ (addon.userDisabled || addon.appDisabled ? " [DISABLED]" : "");
});
strings.sort(nightly.insensitiveSort);
callback(strings.join("\n"));
callback(strings.join(" \n"));
});
} catch(e) {
// old extension manager API - take out after Firefox 3.6 support dropped
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not relevant to this PR but we should get a new issue created so the old API can finally be removed.

@@ -361,7 +361,7 @@ getExtensionList: function(callback) {
+ (addon.userDisabled || addon.appDisabled ? " [DISABLED]" : "");
});
strings.sort(nightly.insensitiveSort);
callback(strings.join("\n"));
callback(strings.join(", "));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you want is to not to call strings.join() here but pass over the array as parameter into the callback. So inside the callback you join the strings then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As said you want to call callback(strings) and do the call to join() where the strings get printed. Sorry but I don't have the time to do any actual work on NTT beside reviews.

@whimboo
Copy link
Contributor

whimboo commented Nov 10, 2015

@UtiluMark do you have an update for us? Would be nice to get this PR finished and merged. Thanks.

@UtiluMark
Copy link
Contributor Author

@whimboo I don't know what to change exactly. Could you finish and merge this PR?

@UtiluMark
Copy link
Contributor Author

@whimboo Could you finish and merge this PR? Then I can continue working on the next PR of #188.

@xabolcs
Copy link
Collaborator

xabolcs commented Aug 20, 2016

@whimboo Could you finish and merge this PR? Then I can continue working on the next PR of #188.

As @whimboo requested, please rewrite the callback argument! Put the .join() in the callback itself!
Write callback(strings); and callback(text) in function getExtensionList!

@UtiluMark
Copy link
Contributor Author

@xabolcs Please check. Thanks.

@xabolcs
Copy link
Collaborator

xabolcs commented Aug 22, 2016

@UtiluMark, thanks for the update. Will review it later.

There is at least two not yet addressed topic here:

@whimboo
Copy link
Contributor

whimboo commented Aug 22, 2016

[remove fallback][1], search for "old extension manager API"

I think that this is perfectly covered via issue #161 already. So please skip this comment.

  • use a preference to [customize the separator][2]

Same here. This was just an idea. I don't see it to be necessary for this PR.

@whimboo whimboo closed this Aug 22, 2016
@whimboo whimboo reopened this Aug 22, 2016
@xabolcs
Copy link
Collaborator

xabolcs commented Aug 22, 2016

@UtiluMark, it looks good to me:

about:addons-memory 12 [DISABLED], BarTab Lite 1.3.1-signed.1-signed [DISABLED], BarTab Lite X 1.9pre, ChatZilla 0.9.92 [DISABLED], Cleanest Addon Manager 7.0.1-signed.1-signed [DISABLED], Developer Profile 1.0.1-signed.1-signed, DOM Inspector 2.0.16.1-signed, Duplicate 2 Window 3.1-signed.1-signed [DISABLED], Extension Auto-Installer 1.2.5, Extension Test 2.16.1-signed.1-let-fixed.1-signed [DISABLED], Flashblock 1.5.20 [DISABLED], Flickr NewTab 1.5.0.1-signed [DISABLED], FlyWeb 1.0.0, MemChaser 0.7.1-signed, MozCamp NewTab 1.6.0.1-signed, Multi-process staged rollout 1.0, New Add-on Bar 0.24, Nightly Tester Tools 3.8, Pocket 1.0.4, Restart 1.2.8, Restartless Restart 9.1-signed.1-signed [DISABLED], Skip Addon Compatibility Check 3.1-signed.1-signed, Sláger on Air! 1.0.20100611 [DISABLED], Tab Counter 1.9.9.3, Tab Groups 0.1 [DISABLED], Tab Mix Plus 0.5.0.1pre.160625a1 [DISABLED], Web Compat 1.0

Thanks!

@whimboo, about that separator idea, could you file an issue? I have ideas too for improvement. :)

@whimboo
Copy link
Contributor

whimboo commented Aug 23, 2016

@whimboo, about that separator idea, could you file an issue? I have ideas too for improvement. :)

I thought more about it and I think that we can do that if there is a demand for it. I don't think we should overload the extension by making too much configurable. We should first make sure to get all services working again. If you think it is very useful with your idea, feel free to file the issue.

@whimboo whimboo merged commit f7e9426 into mozilla:master Aug 23, 2016
@WildcatRay
Copy link

WildcatRay commented Nov 3, 2016

First, I did not expect that I would have to look under Pull Requests for this issue.

The problem is that by making this change, adding a comma between extensions, it means that if you paste into an Excel spreadsheet or a Word document, the list of extensions go into one cell or one paragraph, respectively. Also, if you paste (or insert) into text boxes like this one, you get a "paragraph" as it appears in @xabolcs' comment of Aug 22 just above.

When you have a good number of extensions installed, it is harder to read through a "jumble" like that compared to a "list" of 1 extension per line like mine from my work computer:

about:addons-memory 2016 10.0.1
Add-on Update Checker 2.15
BetterPrivacy 1.77
Classic Theme Restorer 1.5.8.1
Classic Toolbar Buttons 1.5.3
Click to Play per-element 0.3.3
Click-to-Play Manager 1.4.1
Do Not Survey 0.1.1-signed.1-signed
Dorando keyconfig 2016.2
Download Status Bar 15.0.0.1
DownThemAll! 3.0.8
Empty Cache Button 2.7.1-signed.1-signed
Extension Options Menu 2.18
feedly 16.0.528.1-signed.1-signed
Form History Control 1.4.0.6
FoxClocks 4.2.3
Google Reverse Image Search 49.0
Greasemonkey 3.9
HTTPS Everywhere 5.2.6
LastPass 3.3.1
Link Visitor 4.0.1.1-signed.1-signed
Linkification 1.3.8.1-signed.1-signed
McAfee ScriptScan for Firefox 15.3.0 [DISABLED]
More Tools Menu 1.2.4.1-signed.1-signed
Multi-process staged rollout 1.3
Nightly Tester Tools 3.8
NO Google Analytics 0.6.1-signed.1-signed
Places Maintenance 2.0.2
Pocket 1.0.4
QuickPasswords 3.7.2
Reader 48.2
Remove Cookie for Google Account Chooser 1.0.1-signed.1-signed
Saved Password Editor 2.10.1
Self-Destructing Cookies 0.4.11
Session Manager 0.8.1.12
SSL Version Control 0.4.1-signed
Status-4-Evar 2016.10.11.01
Stylish 2.0.7
Tab Mix Plus 0.5.0.1
uBlock Origin 1.9.16
Web Compat 1.0
Yet Another Smooth Scrolling 3.2.6

I am wondering. What is the outlier case here? Is it mine where, prior to this change, you get a list that works well for Excel, Word, and in text boxes on web pages or @UtiluMark's which I am not even sure where it occurs aside, perhaps, if someone is pasting to Notepad.

Might I suggest that the user be offered the option of how they want the list to be provided. The old way where, for many apps, a list is produced and, for users like @UtiluMark, the list with commas after each extension?

@whimboo
Copy link
Contributor

whimboo commented Nov 4, 2016

@WildcatRay thank you for the comment! And sorry that I haven't had the time yet to get all this updated in our issue tracker. So thank you for the reminder.

I filed two more issues where we can track necessary work. Reverting to use new lines should be shipped with the next release, whereby making the separator selectable has no ETA yet.

With that plan I hope we can fix everyones expectations.

@WildcatRay
Copy link

Thank you and your welcome, @whimboo.

@UtiluMark
Copy link
Contributor Author

@WildcatRay @whimboo PR #232 should fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants