Last Comment Bug 156396 - Error Console needs history for the "evaluate" field + some other improvements
: Error Console needs history for the "evaluate" field + some other improvements
Status: RESOLVED WONTFIX
: uiwanted
Product: Toolkit Graveyard
Classification: Graveyard
Component: Error Console (show other bugs)
: Trunk
: All All
-- enhancement (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
http://groups.google.com/group/mozill...
Depends on:
Blocks: 148211 490886
  Show dependency treegraph
 
Reported: 2002-07-08 22:00 PDT by Nicolás Lichtmaier
Modified: 2016-06-29 11:02 PDT (History)
9 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
part 3: enahnce the code evaluator + add the buffer (10.39 KB, patch)
2009-04-30 23:40 PDT, Nochum Sossonko [:Natch]
zeniko: review-
Details | Diff | Splinter Review
patch ver. 2 (16.60 KB, patch)
2009-08-12 19:17 PDT, Nochum Sossonko [:Natch]
no flags Details | Diff | Splinter Review

Description User image Nicolás Lichtmaier 2002-07-08 22:00:30 PDT
It would be useful to be able to access the las recently evaluated strings.
Comment 1 User image Jesse Ruderman 2002-08-07 12:53:31 PDT
http://www.squarefree.com/shell/
Comment 2 User image Nochum Sossonko [:Natch] 2009-04-30 23:39:57 PDT
Gonna drop the third part of the error console patch here, couldn't find a better place :)
Comment 3 User image Nochum Sossonko [:Natch] 2009-04-30 23:40:47 PDT
Created attachment 375297 [details] [diff] [review]
part 3: enahnce the code evaluator + add the buffer

Last part of the initial rework.
Comment 4 User image Philip Chee 2009-05-01 00:14:31 PDT
> +<!ENTITY codeField.emptytext   "Enter some code...">
We now use the UTF8 ellipse character across our codebase instead of three dots.
Also how about "Enter some javascript"? instead. I don't think FORTRAN would work here.
Comment 5 User image Simon Bünzli 2009-05-01 02:42:10 PDT
Comment on attachment 375297 [details] [diff] [review]
part 3: enahnce the code evaluator + add the buffer

So, I'm not sure about these changes, either. Please split this patch in two: one for the buffer (which we want, but likely in a different way) and one for the evaluator.

>+    this.buttonEval = document.getElementById("buttonEval");

What do you need to hold this for?

>+  handleTypeIn: function console__handleTypeIn(aEvent) {
>+    switch(aEvent.keyCode) {
>+      case KeyEvent.DOM_VK_ENTER:
>+      case KeyEvent.DOM_VK_RETURN:
>+        this.evaluate();
>+      break;

Nits: Space after switch; don't indent case; indent break.

>+      case KeyEvent.DOM_VK_UP:
>+        if (this.bufferIndex > 0) {
>+          this.codeField.value = this.buffer[--this.bufferIndex];
>+        }
>+        else {
>+          this.codeField.value = "";
>+          this.bufferIndex = -1;
>+        }
>+      break;
>+      case KeyEvent.DOM_VK_DOWN:
>+        if (this.bufferIndex < this.buffer.length - 1)
>+          this.codeField.value = this.buffer[++this.bufferIndex];
>+      break;

You're re-inventing form history here! Why not just use the implementation we've already got (when available)? You'd even get suggestions; and users could delete individual entries; and entries would persist over sessions; etc.

>+    if (!code)
>+      return window.dump("No Code " + code + "\n");

???

>+    var result = this.frameEvaluator.contentWindow.evaluate(code);

AFAICT this makes it more difficult to debug code errors because the error's column isn't reported anymore (and thus the column marker missing). Also, the error's file is now a "strange" data: URL the user's never seen before... Are the advantages really sufficient for this sacrifice?

>+          src="data:text/html,%3Cscript%3Evar%20evaluate%3Dfunction(aCode)%7Breturn%20eval(aCode)%7D%3C%2Fscript%3E"

Duplicating this data: URL here (from console.js) just makes it more likely that it's forgotten to be changed should it ever have to be updated. Please use about:blank here and set the correct source during gConsole's initialization.

>+<!ENTITY codeField.emptytext   "Enter some code...">

I can already hear some users asking: what code do I have to enter - and what for? ;-)
Please don't make it sound like an order but rather like a statement.

>+#codeField {
>+  margin: 2px !important;

While we're at it: Is !important still required here (also for #evaulateButton)?
Comment 6 User image Nochum Sossonko [:Natch] 2009-05-01 09:58:31 PDT
(In reply to comment #5)
> You're re-inventing form history here! Why not just use the implementation
> we've already got (when available)? You'd even get suggestions; and users could
> delete individual entries; and entries would persist over sessions; etc.

This is quite different from form history where a user can optionally scroll through a list (or get a list of suggestions) and select a value. Here this is a up-down buffer (for lack of a better name) where all you do is hit the up key or down key to change the value of the field. Most developers are used to this type of idea, i.e. firebug has the same thing, as does any decent console (i.e. msys). I don't really see how useful a form history would be here, IMHO...
> >+    var result = this.frameEvaluator.contentWindow.evaluate(code);
> 
> AFAICT this makes it more difficult to debug code errors because the error's
> column isn't reported anymore (and thus the column marker missing). Also, the
> error's file is now a "strange" data: URL the user's never seen before... Are
> the advantages really sufficient for this sacrifice?

I hadn't really thought about that, how important is the column/error information when the error just came directly from code you typed in? It does correctly report the error message which has information explaining what went wrong, I'll look into improving this if it really is a big deal.

> >+#codeField {
> >+  margin: 2px !important;
> 
> While we're at it: Is !important still required here (also for
> #evaulateButton)?

Dunno, will check.

All other comment I'll address and post a split patch.
Comment 7 User image Simon Bünzli 2009-05-01 14:59:03 PDT
I feel that part of the review should also be to challenge your design where I have certain doubts about its justifications. Would you mind taking the time to elaborate slightly further on these two issues:

(In reply to comment #6)
> Here this is a up-down buffer

Indeed. What are its advantages over form history, though? IMO it's not that easily discovered, not that easy to manage, unintuitive in the "we don't do that anywhere else in our own UI" sense and eventually more code to maintain. Why do you think your implementation is still worth it?

> I'll look into improving this if it really is a big deal.

How big a deal it is really depends on what the gains are. Reading through bug 489736 I apparently fail to see the reasons for this change. Doesn't what we currently have work?
Comment 8 User image Nochum Sossonko [:Natch] 2009-05-02 23:48:06 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > Here this is a up-down buffer
> 
> Indeed. What are its advantages over form history, though?

1) No need to press up > enter > enter, form history selects the entry when you hit enter, then "submits" the value on the second enter.
2) the code eval field is somewhat large, I don't think a drop-down list will really look nice on it.
> IMO it's not that easily discovered, not that easy to manage, unintuitive in > the "we don't do that anywhere else in our own UI" sense and eventually more > code to maintain. Why do you think your implementation is still worth it?

It shouldn't be too difficult to maintain IMHO, and unintuitive? I think most developers are pretty familiar with this method of popping/pushing console code, it's how it works in most console environments. It's really just an added feature, with what I consider very little extra code, but let me know if you have a strong opinion about the form history thing, or to totally nix this, it's really up to you :)
> > I'll look into improving this if it really is a big deal.
> 
> How big a deal it is really depends on what the gains are. Reading through bug
> 489736 I apparently fail to see the reasons for this change. Doesn't what we
> currently have work?

Well I thought this was a lot less hacky and easier to maintain. It's very clear to see what is going on (I'm gonna add a comment in which I'll include the decoded text of the data: url), and can be very easily tested. I haven't tried testing the old impl. yet, when I split the patches I'll run the testsuite and see if it passes anyhow.
Comment 9 User image Simon Bünzli 2009-05-03 02:19:28 PDT
In both cases I remain skeptical as to whether your changes' benefits (as stated in comment #8) outweigh their costs (as stated in comment #5 resp. comment #7). Considering that it wont be up to me to decide, however, I'd prefer a Toolkit peer to weigh in, be it through a discussion in mozilla.dev.platform or m.d.a.firefox or through reviewing the split up patches.
Comment 10 User image Nochum Sossonko [:Natch] 2009-05-03 03:07:44 PDT
Posted to dev.platforms, post is in the url field.
Comment 11 User image John J. Barton 2009-05-04 16:31:01 PDT
Some discussion concerning the security of the evaluate function in the Error
Console is on
 Bug 491243 -  Error console command line is insecure
The conclusion was that the Error consule evaluate is secure enough.
Comment 12 User image Nochum Sossonko [:Natch] 2009-05-05 07:27:29 PDT
uiwanted: on the code input idea of this bug, see the post (in the url).
Comment 13 User image Nochum Sossonko [:Natch] 2009-08-12 19:17:50 PDT
Created attachment 394199 [details] [diff] [review]
patch ver. 2

Much improved patch, this addresses the issue of not being able to get a decent url or any good information out of the error thrown by catching and throwing a customized error.
Comment 14 User image Nochum Sossonko [:Natch] 2009-12-23 12:42:52 PST
Pushing out old bugs that I won't have time to actually get to. Feel free to use/extend any patches attached...
Comment 15 User image (Unavailable until Apr 3) [:bgrins] 2016-06-27 07:46:47 PDT
The Error Console has been removed in favor of the Browser Console (see Bug 1278368), and the component is going to be removed.  If this bug is also relevant in the Browser Console, please reopen and move this into Firefox -> Developer Tools: Console.
Comment 16 User image Zack Weinberg (:zwol) 2016-06-27 09:42:18 PDT
I am mass-reopening and re-componenting every single one of the Toolkit:Error Console bugs that appear to have been closed without anyone even *glancing* at whether they were relevant to the Browser Console.

If you want to close a whole bunch of old bugs -- FOR ANY REASON -- it is YOUR RESPONSIBILITY to check EVERY SINGLE ONE OF THEM and make sure they are no longer valid.  Do not push that work onto the bug reporters.

(It's okay to close bugs that haven't been touched in years when they don't have enough information for you to figure out whether the problem is still relevant to the current software - the reporter probably isn't coming back to clarify.  But that is the ONLY situation where it is okay.)

(I'm going to have to do this in two steps because of the way the "change several bugs at once" form works.  Apologies for the extra bugspam.)
Comment 17 User image (Unavailable until Apr 3) [:bgrins] 2016-06-27 15:33:54 PDT
Doesn't translate to browser console, so resolving

Note You need to log in before you can comment on or make changes to this bug.