Error Console needs history for the "evaluate" field + some other improvements

RESOLVED WONTFIX

Status

Toolkit Graveyard
Error Console
--
enhancement
RESOLVED WONTFIX
15 years ago
a year ago

People

(Reporter: Nicolás Lichtmaier, Unassigned)

Tracking

({uiwanted})

Trunk
uiwanted
Dependency tree / graph

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

15 years ago
It would be useful to be able to access the las recently evaluated strings.

Comment 1

15 years ago
http://www.squarefree.com/shell/

Updated

12 years ago
Summary: [RFE] JavaScript console needs history for the "evaluate" field. → JavaScript console needs history for the "evaluate" field.

Updated

11 years ago
Summary: JavaScript console needs history for the "evaluate" field. → Error Console needs history for the "evaluate" field.
(Assignee)

Updated

9 years ago
Product: Core → SeaMonkey
Assignee: hewitt → nobody
QA Contact: jrgmorrison → error-console

Updated

8 years ago
Depends on: 490886
Gonna drop the third part of the error console patch here, couldn't find a better place :)
Assignee: nobody → highmind63
Status: NEW → ASSIGNED
Component: Error Console → Error Console
Product: SeaMonkey → Toolkit
QA Contact: error-console → error.console
Summary: Error Console needs history for the "evaluate" field. → Error Console needs history for the "evaluate" field + some other improvements
Created attachment 375297 [details] [diff] [review]
part 3: enahnce the code evaluator + add the buffer

Last part of the initial rework.
Attachment #375297 - Flags: review?(zeniko)

Comment 4

8 years ago
> +<!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

8 years ago
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)?
Attachment #375297 - Flags: review?(zeniko) → review-
(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

8 years ago
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?
(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

8 years ago
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.
Posted to dev.platforms, post is in the url field.

Comment 11

8 years ago
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.
uiwanted: on the code input idea of this bug, see the post (in the url).
Keywords: uiwanted
Blocks: 148211
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.
Attachment #375297 - Attachment is obsolete: true

Updated

8 years ago
Blocks: 490886
No longer depends on: 490886
Pushing out old bugs that I won't have time to actually get to. Feel free to use/extend any patches attached...
Assignee: highmind63 → nobody
Status: ASSIGNED → NEW
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.
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → WONTFIX
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.)
Status: RESOLVED → REOPENED
Component: Error Console → Developer Tools: Console
Product: Toolkit → Firefox
Resolution: WONTFIX → ---
Doesn't translate to browser console, so resolving
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Component: Developer Tools: Console → Error Console
Product: Firefox → Toolkit
Resolution: --- → WONTFIX
(Assignee)

Updated

a year ago
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.