394589 - Remove / change max source tab pref

Status: RESOLVED FIXED

(Other Applications Graveyard :: Venkman JS Debugger, defect)

Description

[:Gijs (he/him)]

STR:

1. Attempt to open more than 5 source files in Vnk.

AR:
It won't work without changing the max source tabs pref

ER:
Venkman opens more than 5 source files.


I vote we ditch the pref or at least change the default to be more sensible.

Comment 1

[Karsten Düsterloh]

To be more precise: if you open the sixth source file, the first source view will be dropped. This is quite surprising, I almost doubted my memory. ;-)

Comment 2

[:Gijs (he/him)]

Attachment: Patch

- Change pref to 10
- Display a message if we load source into an existing tab. Should be helpful the first time, and not too disturbing the other times. I'll take suggestions that help balance this out, as long as it doesn't involve alerts combined with checkboxes and prefs and such (or would that be better? I'm not sure...)

Comment 3

[Alex Vincent [:WeirdAl]]

Comment on attachment 281801 [details] [diff] [review]
Patch

>Index: mozilla/extensions/venkman/resources/content/venkman-views.js
>     else
>+    {
>         index = 0;
>+        display(getMsg(MSN_MAXTABS_REACHED, maxSourceTabs), MT_INFO);
>+    }

What's going to happen if the user opens tab 11, gets this dialog, clicks OK, and then tries to open yet another file?  From my reading of it, you'll display the message again when they only need to be told once.

>Index: mozilla/extensions/venkman/resources/locale/en-US/venkman.properties
>+msn.maxtabs.reached  = The maximum number of source views (%1$S) has been reached. Alter the ``source2View.maxTabs'' preference if you want to change this limit. Source will now load in an existing tab.

This doesn't look quite right.  Double-backticks, and double single quotes...  It should be double quotes, single single quotes, or removed.

Comment 4

[:Gijs (he/him)]

(In reply to comment #3)
> (From update of attachment 281801 [details] [diff] [review])
> >Index: mozilla/extensions/venkman/resources/content/venkman-views.js
> >     else
> >+    {
> >         index = 0;
> >+        display(getMsg(MSN_MAXTABS_REACHED, maxSourceTabs), MT_INFO);
> >+    }
> 
> What's going to happen if the user opens tab 11, gets this dialog, clicks OK,
> and then tries to open yet another file?  From my reading of it, you'll display
> the message again when they only need to be told once.

It's not a dialog, it's an info message displayed in the console.

> 
> >Index: mozilla/extensions/venkman/resources/locale/en-US/venkman.properties
> >+msn.maxtabs.reached  = The maximum number of source views (%1$S) has been reached. Alter the ``source2View.maxTabs'' preference if you want to change this limit. Source will now load in an existing tab.
> 
> This doesn't look quite right.  Double-backticks, and double single quotes... 
> It should be double quotes, single single quotes, or removed.

This notation is special. It will get parsed by the message manager as such. The other strings use it too (see the context).

Comment 5

[Alex Vincent [:WeirdAl]]

Comment on attachment 281801 [details] [diff] [review]
Patch

(In reply to comment #4)
> It's not a dialog, it's an info message displayed in the console.

Still, once should be enough.  But if it's not a distraction, then r=ajvincent.

Comment 6

[:Gijs (he/him)]

Attachment: Slightly better patch

Using confirmEx to pop up a dialog with a checkbox instead. Also allow "infinite" number of tabs by setting the preference to 0, and using real quotes now because the message ends up in a dialog (where the munger can't work its magic).

Comment 7

[Alex Vincent [:WeirdAl]]

Comment on attachment 281859 [details] [diff] [review]
Slightly better patch

>Index: mozilla/extensions/venkman/resources/content/venkman-utils.js
>+function confirmEx(msg, buttons, defaultButton, checkText,
>+                   checkVal, parent, title)
>+    var buttonConstants = {
>+        ok: ps.BUTTON_TITLE_OK,
>+        cancel: ps.BUTTON_TITLE_CANCEL,
>+        yes: ps.BUTTON_TITLE_YES,
>+        no: ps.BUTTON_TITLE_NO,
>+        save: ps.BUTTON_TITLE_SAVE,
>+        revert: ps.BUTTON_TITLE_REVERT,
>+        dontsave: ps.BUTTON_TITLE_DONT_SAVE

nsIPromptService.*, not ps.* - yeah, I know ps provides them too, but let's refer to the interface and not the component.

>+    if (!isinstance(buttons, Array))
>+        throw "buttons parameter must be an Array";
>+    if ((buttons.length < 1) || (buttons.length > 3))
>+        throw "the buttons array must have 1, 2 or 3 elements";

I'd suggest moving these checks to the very beginning of the function (i.e. before getting the prompt service).  Thus, if we have bad arguments, we do less work all around.

>+        var buttonFlag = ps.BUTTON_TITLE_IS_STRING;
>+        buttonFlags += ps["BUTTON_POS_" + i] * buttonFlag;
>+        buttonFlags += ps["BUTTON_POS_" + defaultButton + "_DEFAULT"];

nsIPromptService again.

>Index: mozilla/extensions/venkman/resources/content/venkman-views.js
>+            confirmEx(getMsg(MSN_MAXTABS_REACHED, maxSourceTabs), ["!ok"], 0,
>+                      MSG_MAXTABS_ALERT, checkVal);

I understand what you're doing, and I think ultimately you're calling the wrong method of nsIPromptService given the single use case here.  One button, not checking the return value from the service, and it's not clear to me why you have the ! character in the button.  It looks to me nsIPromptService.alertCheck() is more suitable here.  Copying code simply because it's used elsewhere isn't quite a good enough rationale for a very first use.

If you plan on using confirmEx in other parts of Venkman code for real multi-button values, and checking the return value, then I'd buy this without question.  It's not enough justification for me to explicitly r- this patch right now - really, that depends on your intended uses of confirmEx in the future.

Comment 8

[:Gijs (he/him)]

Attachment: Patch using alertCheck

Good point. Let's use alertCheck instead.

Comment 9

[Alex Vincent [:WeirdAl]]

Comment on attachment 281865 [details] [diff] [review]
Patch using alertCheck

>Index: mozilla/extensions/venkman/resources/content/venkman-utils.js
>+    if (!title)
>+        title = MSG_ALERT;

Where is MSG_ALERT defined?  LXR shows it only for IRC, and not very localized...

r=ajvincent with this nit answered.

Comment 10

[James Ross]

(In reply to comment #9)
> Where is MSG_ALERT defined?  LXR shows it only for IRC, and not very
> localized...

http://mxr.mozilla.org/seamonkey/search?string=msg.alert&find=mozilla%2Fextensions%2Fvenkman&findi=&filter=&tree=seamonkey

Fear the Message Manager and its shroud of mystery.

Comment 11

[Karsten Düsterloh]

(In reply to comment #2)
> - Change pref to 10

Did you find out why this still very arbitrary restriction is there in the first place? What goes wrong if you drop it entirely?

Comment 12

[James Ross]

FWIW, my educated guess is simply that the tabs squash to unreadable levels at much more than 5 (on a typical system back in 2002 when the pref arrived).

Comment 13

[:Gijs (he/him)]

(In reply to comment #12)
> FWIW, my educated guess is simply that the tabs squash to unreadable levels at
> much more than 5 (on a typical system back in 2002 when the pref arrived).
> 

That's what I thought as well. By now though, we can assume people have larger screens and will be less bothered. :-)

And yeah, Alex, the message manager automagically defines constants for strings in the venkman.properties file starting with "msg." and "msn.", where all dots become underscores and the string is capitalized.

Not very interesting, but the reason it's defined for irc separately is that the file it's in (utils.js) is used by other users of the irc library (mingus) and they don't have the localization stuff, and hence need the constants around because some of the utility functions in utils.js are called from the bot and they expect those constants to be around.

Comment 14

[:Gijs (he/him)]

Checking in mozilla/extensions/venkman/resources/content/venkman-utils.js;
/cvsroot/mozilla/extensions/venkman/resources/content/venkman-utils.js,v  <--  venkman-utils.js
new revision: 1.36; previous revision: 1.35
done
Checking in mozilla/extensions/venkman/resources/content/venkman-views.js;
/cvsroot/mozilla/extensions/venkman/resources/content/venkman-views.js,v  <--  venkman-views.js
new revision: 1.39; previous revision: 1.38
done
Checking in mozilla/extensions/venkman/resources/locale/en-US/venkman.properties;
/cvsroot/mozilla/extensions/venkman/resources/locale/en-US/venkman.properties,v  <--  venkman.properties
new revision: 1.66; previous revision: 1.65
done