75836 - directory viewer should display ftp control log

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Doug Turner (:dougt)]

the directory view should be displaying the control connection log.  Most FTP 
applications do some sort of logging.  This will also allow more indepth user 
feedback.

Comment 1

[Doug Turner (:dougt)]

Attachment: changes to components/directory

Comment 2

[Doug Turner (:dougt)]

attach 30680 includes:

nsDirectoryViewer.cpp
1. Added nsIPrompt and nsIAuthPrompt interface to the directory viewer so that 
problems can be reported to the enduser
2. Implemented nsIFTPEventSink to recieve ftp control connection logging
3. Attempting to reduce/serialize network requests

directory.js
1. renamed trans to transferable to avoid confusion

directory.xul
1. added hidden splitter and box so that logging can be viewable.

Various makefile changes.

Comment 3

[Doug Turner (:dougt)]

Attachment: Changes to themes/

Comment 4

[Doug Turner (:dougt)]

The last diff (30683) sets color to the different div classes which the 
directory view adds.

Comment 5

[Chris Waterson]

Couple hen-pecks. But top-line comments are

- Will this break bbaetz's gopher stuff? Or just be hella ugly for
  gopher URLs?

- You're turning *on* Geek Information Central by default, without any
  clear way to hide it. I'd kinda think you'd want to have it *off* by
  default.

- Can you do the DOM manipulation in JS? All the interfaces you need are
  XPConnected, right? Make it easier for mortals to change this stuff
  (and beautify it) later.

     content/communicator/directory/directory.xul               
(directory/directory.xul)
+
content/communicator/directory/directory.html              
(directory/directory.html)
 

Fix hard tabs.

+  PRBool mLogWindowEnabled;
+  nsCOMPtr<nsIDOMDocument> mDocument;
+  nsCOMPtr<nsIDOMWindowInternal> mLogWindow;
+  nsCOMPtr<nsIDOMElement> mLogDivElement;

Did you verify that this doesn't introduce any new leaks?

Do you really need to do all this stuff in C++? Can't you implement JS objects
that'd do all the nasty DOM manipulation?

+    nsCOMPtr<nsIDOMElement> elem;
+    treeDoc->GetElementById(NS_LITERAL_STRING("main-splitter"),
getter_AddRefs(elem));
+    if (!elem)
+      return NS_OK;
+    
+    elem->SetAttribute(NS_LITERAL_STRING("hidden"), NS_LITERAL_STRING("false"));

So, like, I don't want to see this. I don't think many people will. How do you
deal with persisting it's ``off''-ness

         mDirRDF->GetResource(NC_NAMESPACE_URI "loading", &kNC_loading);
-        mDirRDF->GetResource(NC_NAMESPACE_URI "URL", &kNC_URL);
+    mDirRDF->GetResource(NC_NAMESPACE_URI "URL",     &kNC_URL);

Warning, Will Robinson! Tab kookification!

+          char* uri = nsnull;
+          if (aSource) uri = httpIndex->GetDestination(aSource);
+          
+          nsresult            rv = NS_OK;
+          nsCOMPtr<nsIURI> url;
+          
+          if (uri) {
+            rv = NS_NewURI(getter_AddRefs(url), uri);
+            nsMemory::Free(uri);
+          }

nsXPIDLCString-ify?

+    if (refireTimer == PR_TRUE)
+    {

C'mon, do you really want your name in CVS blame on this? How about just

  if (refireTimer)

Comment 6

[Bradley Baetz]

My stuff should be fine, as long as dougt turns the logging off for gopher (not
that there's anything to log). And if he's tested a gopher url or two :)

I don't see what you're doing with the nsIPrompt stuff - I'm popping up a prompt
with no problems at all (bug 70267 - I have to finalise my fix for 70529 before
I check that in)

waterson - the GetDestination stuff is cleaned up in 70267, which you sr'd and
jag has some nits for me to clean up. I'll finish that sometime tomorrow.

Comment 7

[Doug Turner (:dougt)]

- Will this break bbaetz's gopher stuff? Or just be hella ugly for
  gopher URLs?

No.  The logging box/window/pane will not be displayed.  BUT!!! damn, I sent you 
a diff that does not have the check.  new patch coming soon....

- You're turning *on* Geek Information Central by default, without any
  clear way to hide it. I'd kinda think you'd want to have it *off* by
  default.

Okay.  If I have the splitter in the closed position by default would that be 
okay?

- Can you do the DOM manipulation in JS? All the interfaces you need are
  XPConnected, right? Make it easier for mortals to change this stuff
  (and beautify it) later.

I could do most, if not all, of the DOM manipulation in JS.  The only two things 
that I am doing is unhiding the loggin window when I know that it and adding 
text to a div, both of which aren't really intented to be changed via 
non-mortals. 


Bradley, the nsIPrompt stuff causes me alot of grief.  If the directory viewer 
opens a channel, it passes the httpIndex listener, and also uses it's 
nsIInterfaceRequestor.  Prior to this patch, there is not any 
nsIInterfaceRequestor impl on the httpIndex's listener.  If a protocol need to 
prompt during this load, it will fall on deaf ears.  This happens alot with ftp 
if you revisit a site that has many open directories in the tree widget.  Now, 
since I am adding the nsIFTPEventSink as a interface that can be requested, I 
should fix the nsIPrompt stuff.  

It leaks like a sieve.  I will fix that. patch two coming soon...  


but first, chris, tell me if poking the dom from C is okay for this.  

Comment 8

[Doug Turner (:dougt)]

Attachment: Changes to xpfe/components

Comment 9

[Doug Turner (:dougt)]

Last patch:

Resolve refcount cycles.  Chris thanks for your help...  
Moved DOM poking into JS.

Comment 10

[Doug Turner (:dougt)]

Bradley, can you review the last patch.
Waterson, can you sr' it?

Comment 11

[Bradley Baetz]

You didn't end up merging my patches - I'll do it later then.

+               // Lets also enable the loggin window.

typo :) whitespace.

Is it possible for all the OnFTPControlLog stuff to be done entirely in js? You
can make a js object implement the interface (jag pointed me to navigator.js
nsXULBrowserWindow as a good example), but I'm not sure how (or if) you can get
this to happen from ::GetInterface. waterson? jag?

-        tree.database.AddDataSource(HTTPIndex.DataSource);
+        tree.database.AddDataSource(HTTPIndex);

What is this meant to do?

Also, the user can drag the splitter up, and the log only covers part of the
window - the rest is that wonderful blue color :). It should also probably
persist the splitter position - as it is, going forward then back (or going to
another ftp page) turns the splitter off.

You got rid of the Add/DeleteRef pair for the httpindex object - is the comment
that you deleted simply not valid now?

And some ftp problems:

The loading animated gif isn't being displayed until just before all the data is
sent - the onstartrequest isn't being fired until then, even though the logging
window is being updated. This appears to be an ftp regression (it happens
without your patches, and doesn't happen with gopher) - want me to file a bug?

I also see that the control connection isn't being cached all the time. As well,
multiline responses are all being displayed on one to two lines in the logging
window.

Comment 12

[Doug Turner (:dougt)]

> You didn't end up merging my patches - I'll do it later then.

I can do it, I just didn't want to do mingle the patches.

> Is it possible for all the OnFTPControlLog stuff to be done entirely in js? 

I could.  It would involve forking off the GetInterface to JS.  I will pass on 
this for now.  

-        tree.database.AddDataSource(HTTPIndex.DataSource);
+        tree.database.AddDataSource(HTTPIndex);

> What is this meant to do?

It is the same thing.  the HTTPIndex IS the datasource.  (thanks to waterson for 
pointing this out!)


>Also, the user can drag the splitter up, and the log only covers part of the
window - the rest is that wonderful blue color :). It should also probably
persist the splitter position - as it is, going forward then back (or going to
another ftp page) turns the splitter off.

Worse is better.  I am sure that someone with better XUL hackin knowledge can 
fix it.  

>You got rid of the Add/DeleteRef pair for the httpindex object - is the comment
that you deleted simply not valid now?

Correct... I think that waterson has a comment that he wanted to add.  

> And some ftp problems:

Seperate known bugs.


With all of this being said, can I get an r=?

Comment 13

[Doug Turner (:dougt)]

>Also, the user can drag the splitter up, and the log only covers part of the
window - the rest is that wonderful blue color :)

OH!  I misread this.  the iframe needs to strech.

Comment 14

[Bradley Baetz]

> OH!  I misread this.  the iframe needs to strech.

Yeah. Fix that, add a persist="collapsed" to the box object in the xul (I think
thats all you need - I can't test because my tree is rebuilding) and r=bbaetz

Comment 15

[Doug Turner (:dougt)]

Fix checked in.  Lets see how people like/dislike it.

Comment 16

[Blake Ross]

>> Also, the user can drag the splitter up, and the log only covers part of the
>> window - the rest is that wonderful blue color :). It should also probably
>> persist the splitter position - as it is, going forward then back (or going 
>> to another ftp page) turns the splitter off.

> Worse is better.  I am sure that someone with better XUL hackin knowledge can
> fix it.  

Thanks for fixing it.  For the future, this is not an acceptable answer.  If 
you're touching XUL, you need to know what you're doing with it.  `Someone who 
knows more about it will fix it later' isn't an acceptable answer, and has 
resulted in what our UI is today.  I thought Ben's e-mail a few weeks ago made 
all this clear...

Comment 17

[Doug Turner (:dougt)]

I didn't read Ben's email.  Didn't mean to suggest that I would completely pass 
the buck, but I do not believe that everything we built must be 100% polished 
when it is first checked in.  Worse is Better.  

Comment 18

[sairuh (rarely reading bugmail)]

vrfy fixed using 2001.04.18.0x comm bits. however, the grippy to display the log
info needs to be clicked first before it's draggable --filed bug 76584 for that.