Closed Bug 74121 Opened 23 years ago Closed 23 years ago

Clean up the new Properties window

Categories

(SeaMonkey :: General, defect)

Product:
SeaMonkey
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla0.9.1

People

(Reporter: hwaara, Assigned: gerv)

References

(
URL
)

Details

(Keywords: meta)

Attachments

(7 files)

First try at cleanup
17.13 KB, patch
Details | Diff | Splinter Review
Second go - a bit more expansive
31.62 KB, patch
Details | Diff | Splinter Review
Patch version 3
35.44 KB, patch
Details | Diff | Splinter Review
hreflang test case for lang attribute parsing code
675 bytes, text/html
Details
Patch v.4 (with hreflang changes; new regions; more cleanup)
25.93 KB, patch
Details | Diff | Splinter Review
Patch v.5 - tab removal, entity renaming. r=rbs
29.60 KB, patch
Details | Diff | Splinter Review
diff -bw for alecf
22.89 KB, patch
Details | Diff | Splinter Review 
We discussed this on IRC and this is what the general conclusion was:

* Remove the "Properties" context-menuitem if the click was not on a link/image
or some other supported metadata-type. (We already have got Page Info for that)
Once bug 52730 is fixed we will have an even better pageinfo dialog.

* If the click was on an image, then display the context-menuitem as "Image
Properties", if it was on a link then "Link Properties" and so on...for all
supported metadata-types.

* Remove the document-wide groupbox and its information, this is redundant (once
again, we have pageinfo) and it's not what the user possibly wants. And if it
wants that info, then it shouldn't go there in the first place.  So, only show
relevant data about the current metadata-type.  Possibly have a link/button to
the pageinfo dialog in case the user wants that info (this is yet unknown if it
will be implemented or not), but focus on the above three issues *first*.
A few other things:

* the URL text should be wrappable and not make the window very, very wide if
the URL is long.

* The window should have a title. For example, "Image Properties"
Also: 
- "Miscellaneous" is spelt like that, and not as in the dialog :-)
- Title attributes displayed in the Properties box seem to acquire a number of 
newlines - they should be as wide as the containing box. See the first title in 
the EvilTests page for an example.
- Properties should have a capital P.
- You need some whitespace (or greyspace) between your property names and the 
values.
- Can we convert text-languages attributes to the actual name of the language, 
using a lookup table? "en" is ugly.

But good work :-)

Gerv
URL: http://www.bath.ac.uk/%7Epy8ieh/inter...
First of all, I currently have no build environment so I can't fix any of this
right now, will hopefully get a new env in a week or so.

I don't like the idea of renameing the window to image-properties and
link-properties depending on where you have clicked. What if the user clicks an
image that is also a link?

The newlines in titles seems to be a reflow bug in XUL. I'll file a bug when I
get my environment back (unless someone beats me to it, hint, hint)

The problem with showing the context-menu-item only when metadata is avalible is
that It requires lots of digging around in the document and therefore could make
bringing up the context-menu slow. I'll investigate.

I totally agree on the "en"->"English". I just wanted to get the properties
window in there to get feedback on it before I did the RightThing everywhere.
I'll investigate
Status: NEW → ASSIGNED
You don't need a build environment to work on this. Merely download and install 
a Mozilla, unjar all the chrome (mail me if you need help with this) and then 
work away. Then, you can just copy the files back into the right places in your 
build tree to make the diffs.

I agree about the Image/Link properties thing, but the window should have _some_ 
title (Properties, I suggest.) You also need to do the i18n ASAP, and certainly 
before 0.9. If you want me to do this, let me know, but I need things to 
stabilise first.

Working out whether you are on a link or an image is easy. There are isLink and 
isImage variables calculated for you. Look at how the "View Image" link is 
displayed (or not) for an example. Then, you can make the visibility of the 
Properties menu item depend on one of those two booleans being true.

Gerv
I don't even have a computer connected to internet :) (other then at school
where I can't even run moz).

what i18n issues are you talking about? The "en"->"English" stuff? in that case
I don't know if moz has such code somewhere so I would appritiate any help.

I thought that the window did have a caption that said "Properties" but it could
have dissapeared in the xul-syntax-change, I'll fix it asap.
Hardware: PC → All
I suppose I mean l10n rather than i18n. What I mean is moving the strings out to 
the DTD file, which I... no, hang on. That's Page Info. Sorry to bother you. 
Don't mind me. Nothing to see here. Move along.

If you can't work on this, can I have a go? :-)

Gerv
As I said in bug 74729, I suggest having a bunch of `Info' windows -- `Page 
Info', `Frame Info', `Image Info', and `Object Info'. They should all have the 
same look (width, height, etc). I don't think the `Link Info' window should be 
retained, since it doesn't provide any useful information -- the URL is already 
shown in the status bar on mouseover, and the link target could be better 
indicated by fixing bug 9805.

Additional note (which may need a new bug blocking this one): the title for an 
Info window should be `Info for test.png' or whatever. This requires chopping 
off the last part of the URL, or the last hierarchy if the URL ends in `/'.
Depends on: 74729
Keywords: meta
Sorry, indicating the link target is bug 14027 (not bug 9805).
See my comments in bug 74729, but the problem with your "different titles" 
proposal is that what happens when you have an image which is also a link? Or an 
image which is a link inside a BLOCKQUOTE? "Properties" is the only sane title 
for that dialog, and it has to be a combined dialog unless you want three of 
them to pop up in some circumstances.

Gerv
OK, patch coming (when my cvs update finishes and I merge) which addresses all 
the issues raised in this bug, with the exception of changing the window title 
in an element-dependent fashion, and making the URL and other text lines text 
wrap. This includes language abbreviation lookups, which now work.

General Notes:

bug 72522 is now fixed, so the code that needed it fixed should now be changed 
to use Node.baseURI. I don't know how to do this.

We are crawling the parent tree 3 times in showMiscInfo (metadata.js), and once 
each in the other showFooInfos. Would it not be more sensible to rejig this to 
crawl once, and passing each node it finds (that is an ELEMENT) to five 
functions (one for each box) which check whether that particular node is one it 
is interested in? This would save a lot of operations.

Notes on upcoming patch:

metadata.js

The algorithm which decides whether to show the Properties menu item does not 
check every possible property. Jonas is right - this would be too slow. Instead, 
we merely check whether we are over either A, IMG, BLOCKQUOTE, INS, DEL or Q, or 
whether we have a title or lang. This, I think, is a good compromise and will 
get it right 99% of the time. If it gets it wrong, the Properties window will 
simply say "No properties set." Comments on this solution welcome.

Now that we only show the Properties menu item when we actually are over an 
Element of some sort, the test at line 58 indicates a programming rather than a 
user error. So, instead of an alert, should we have a dump?

Please check my comment added at line 137 is correct.

nsContextMenu.js

I've done some reasonably serious rearrangement for efficiency reasons - we now 
only crawl the parent chain once (as suggested above) looking for links, 
metadata elements and text boxes.

General

When we click a URL, Properties used to hang around while the page underneath 
changed. I've implemented window.close() on link click, but the other option is 
opening in a new window instead. Discuss.

I've fixed the problem with title text wrapping too much - it was caused by 
using <html> instead of <text>. However, this text (like URLs) now doesn't wrap 
at all. We need to find a XUL expert who understands how text wrapping works.

Gerv

    
    

    
  
There it is. Jonas - can I have your review first before we go chasing r= and 
sr=? 

Also, we need to discuss the points I raised above, and also what accesskey we 
should choose (if any.) You were right - the question mark appears as:
Properties(?)
which makes us look indecisive :-)

Gerv

    
    

    
  
Comments from looking at the patch:

* I don't think you should add the metadataCmd.accesskey entityrefernce without
  adding the entity. Could you use "" as entity value so that it could be added
  in case someone finds a good key? (or in another locale)
  
* The whitespace should be added in the metadata.xul, not in metadata.dtd

* I think we can afford looking at the attributes as well in nsContextMenu.js
  so that risk of an "false" properties menuitem is reduced

* You need to look for table summaries aswell in nsContextMenu.js (are there
  anything more?)

* Keep the old unableToShowProps item and make a new noPropsAvalible for the new
  message

* It would be a bit cleaner if you initated gLangBundle where gMetadataBundle is
  initiated

* Don't use <text> instead of <html> for titles. I used <html> so that the text
  would wrap. I *think* that's how you get text to wrap in xul

Other then that this looks fine. Reassigning to you
Assignee: sicking → gervase.markham
Status: ASSIGNED → NEW

    
    

    
  
Done all that, except:

> * Keep the old unableToShowProps item and make a new noPropsAvalible for 
> the new message

Er... they are the same message :-) That is, they are used in the same 
circumstance. So they can't co-exist.

> * Don't use <text> instead of <html> for titles. I used <html> so that 
> the text would wrap. I *think* that's how you get text to wrap in xul

Well, using <html> does get the text to wrap, but it makes the titledbox go 
wrong. We may have found a bug here. Also, we want the links to wrap too, and we 
can't make them <html>, or they lose their clickability (for some reason.)

More patches soon...

Gerv

    
    

    
  
OK... I've rather gone mad with this. I refactored it... and it now looks rather 
different. It's definitely more efficient, though. :-) The diff has suddenly got 
such that it probably makes more sense to check the whole file... Anyway, 
attaching for people to have another look.

Gerv

    
    

    
  
I don't like the changes to metadata.js. Since speed isn't critical for this 
code I'd much rather that the code is kept simple and clean rather then speed 
optimized. sorry...

IMO unableToShowProps is displayed when an error has occured whereas 
noPropsAvalible are shown when we have errorously shown the Properties 
contextmenu item. So we need both (or preferebly only the first)

The strange wrapping and unclickablity of <html> is a bug and should be filed 
as such.

    
    

    
  
> I don't like the changes to metadata.js. Since speed isn't critical for 
> this code I'd much rather that the code is kept simple and clean rather 
> then speed optimized. sorry...

Do you think the new version is less simple and clean than the old? It's just a
conceptual change - instead of the functions checking the entire tree they now
check one node per call. Functions like the InsDel checker are now down to 5
simple lines instead of 15. The code which does stuff is no longer inside 3 if
statements. 

I dispute your claim that speed isn't important. Mozilla needs all the speed it
can get :-) In the patch, we no longer call isHTMLElement five thousand times
per menu item on the same string. We no longer check that a node is an element
node redundantly about five thousand times either. We walk the parent tree 3
times instead of eight or nine. We don't make function calls unless we still
haven't found an element of that type. This has all got to make a difference -
function calls are expensive.

My point about unableToShowProps and noPropsAvailable is that I have replaced
one with the other, i.e. they are both the same piece of code. If you think that
there are two error situations here which need flagging differently, we'll have
to work out a test for each of them.

I agree about the wrappability of <html>, although I think the unclickability is
possibly something to do with the styling? 

Gerv



    
    

    
  
No I don't think the new code is cleaner. Even though it is fewer lines the 
current flow in the code is IMO better. Each showXInfo function display the 
avalible metadata for an element, this isn't so bound to walking up in the tree 
but would allow much more crosswalking in the document if needed in the future. 
Your new code contains much more global flags and global logic which is a bad 
thing.

Speed really isn't an issue, it dosn't matter if the properties window takes 
0.25 or 0.05 seconds to open. Show me a page where the window is really slow to 
convince me.

The message unableToShowProps should be shown the code can't find the element 
to show metadata for (just in case...) the current code looks like this:

if (!elem) {
    alert(gMetadataBundle.getString("unableToShowProps"));
    window.close();
}

which I think should stay (although maybe something neater then an alert could 
be used...)

    
    

    
  
Sicking, speed is always an issue in terms of Mozilla. Don't argue otherwise.

In the future someone might want to extend the properties dialog, and if s/he
does then s/he shouldn't have to rewrite the current code because the original
authors didn't care about speed.

If the patch's code isn't obscurely hard to read and it functions/behaves
exactly the same as the old code then please use it.

    
    

    
  
Gervase:

First of all, this is my personal oppinions, you are perfectly free to go chase 
an r= even if I don't like all your changes. This all comes down to that I 
don't like your new structure of metadata.js since IMO it contains too much 
global logic and isn't as flexible as the old structure. However this is a very 
subjective view and I'm not saying that it's the only valid view.

Don't take this as a flame on your code. I do like the changes in the first 
patch and if you change the things I've commented on I'd be very happy to add 
my r= to the code (uhm if i'm allowed to r= stuff, I don't know all reviewing 
rules)

Håkan:

Mozilla needs speed, but does the properties window? Please show me a page 
where the changes gives a noticeable performance increase.

Also, I would sacrifice execution speed for code cleanness, if the speed loss 
is small enough and the cleanness gain is big enough. There is IMHO such a 
thing as "not timecritical code" (how much performance work do you think is 
done in XPInstall?). However this is my personal view of how coding should be 
done and I don't want to force that oppinion on you.

Regarding future changes: Future changes is exactly the reason that I want to 
keep the current structure, I'd much rather keep code easy to change then speed 
optimized if the code is "not timecritical" which I beleave is the case for 
this code.


    
    

    
  
*** Bug 75078 has been marked as a duplicate of this bug. ***

    
    

    
  
comments below are from gervase.markham@univ.ox.ac.uk. Sent to me through email.

-------

Well, I don't want to ride roughshod over your feelings seeing as you wrote the 
original code, but I do thing mine is better (otherwise I wouldn't have written 
it ;-). How about we come to an amicable solution: we find a reviewer to 
compare the two approaches, asking him to discount the other incidental 
improvements I made, and say which he thinks is the better approach. If he says 
yours, I'll gladly do a new patch with just the other stuff. If he says mine, 
we'll check mine in. That way, we have an independeny judgement from a strong 
hacker. How does that sound?

>my r= to the code (uhm if i'm allowed to r= stuff, I don't know all reviewing 
>rules)

If you have checkin access to the tree, you can give r=. I was more asking for 
an unofficial one because I wanted to see what you thought. Now I know :-)

>Regarding future changes: Future changes is exactly the reason that I want to 
>keep the current structure, I'd much rather keep code easy to change then 
speed 
>optimized if the code is "not timecritical" which I beleave is the case for 
>this code.


Adding future changes to my structure is easy - each new metadata item requires 
a new function to check for it, and a call to that inside the main search loop. 
It's very modular.

Gerv

    
    

    
  
*** Bug 75693 has been marked as a duplicate of this bug. ***

    
    

    
  
Hate to interject after all this coding, but it seems like many applications 
have a Properties dialog just about everywhere they possibly can, and users are 
coming to understand and expect this.  Given that, shouldn't the Properties 
window always show up, and if you don't find a property, just bring up 
the "Page Info" dialog?

The code written will still be useful to determine where the Properties dialog 
goes, and looks like a big improvement in terms of how the actual Properties 
dialog looks ... but Page Info is really Properties for the page (and 
Properties for the page is really Page Info).

    
    

    
  
I think that would confuse users. There's a clear semantic difference between the sort of properties that relate to page elements, and those which relate to the actual page (this is why HTML documents have a HEAD section and a BODY section.) Having one dialog or the other for the same menu item according to where you clicked would definitely break the principle of least surprise. 

sicking: It would be good to hear your thoughts on my proposal for the future of my patch. Is that acceptable? If so, we'll head off and find a reviewer.

Gerv



    
    

    
  
Gerv: Sounds like an excellent idea. I was waiting for you to show up in 
#mozilla and I guess that'll be around the 17'th?

    
    

    
  
It would be great if the image dimensions and (even more important) their size
(content-size) were on the dialog. I know that you can get their dimensions from
page info, but for that you must look for an image in a (sometimes very) big
list. It would be a lot easier to just right-click at them.
The content-size is another story. There is no way to get that information
without actually saving it and checking with another program. Web developers
(well, at least I) look for this information all the time and... well, wouldn't
it be a shame if I needed to use IE? :(

    
    

    
  
Don't use this bug to request new features, file separate RFE bugs for that. 
But you can rest asure that the properties window (or whereever the information 
will end up) will contain more information eventually. When I originally 
designed the properties window I did so with the IE properties window in mind.
Summary: Clean up the new properties window → Clean up the new Properties window
Depends on: 77898

    
    

    
  
OK, I'm attaching a new patch, because the old one had rotted a bit. As a bonus,
the new one contains an implementation of Hixie's spec for converting lang
attributes to text, and some new languages for the properties file (from the
latest version of the spec.)

Still looking for r=.

Gerv

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch version 3
        — 
          — Splinter Review
      

    


  

    
    

    
  
+<!ENTITY cutCmd.accesskey             "u">
Cut is Cu&t in windows, i'm not sure about mozilla.

    
    

    
  
mind putting the entity list in alphabetic order or something?

    
    

    
  
I think they are currently grouped logically, by the section they refer to - why 
would you like them in alphabetical order? :-)

Gerv

    
    

    
  
I guess what I had in mind was the use of _consistent_ prefixes that will just 
cause the ordering to fall through as a by-product:

document-*
image-*
link-*
etc...

    
    

    
  
Things to fix: the entity list and the messy tabs (space anywhere and everywhere 
should be preferred). And I will give my r=rbs.


    
    

    
  
alecf: Could you sr= this, please? You should have an email with the details.

Gerv

    
    

    
  
I'm missing the changes to nsContextMenu.js. Or has that already gone in?

    
    

    
  
Yes, they have. In the patch for bug 25071. Aargh! Are reviewers not supposed to
save my from my own stupidity? Ah well, if it's reviewed and super-reviewed, it
must be OK, right? Right?

Hmm. I'm pretty sure this won't make much difference, but we should try and get
Patch v.5 checked in soon to avoid any possible problems. Alecf is away until
Tuesday...

Gerv

    
    

    
  
Hey Gerv, I did said that I will stamp the 'r=rbs' myself (which leaves the
the opportunity to change my mind if a better idea was found in the meantime
or for some other reason to my discretion...)
OK, had a look at the new patch. It addresses my remarks. r=rbs.

    
    

    
  
rbs: I apologise. It is a common idiom (see other bugs) for reviewers and 
super-reviewers to say "Fix this and you have r=mozbot", and I assumed you were 
doing the same thing.

Gerv

    
    

    
  
No worries then if this is a confined (but well-established) practice in 
bugzilla.

In fact, apart from these changes of typographical nature, I liked the general 
structure that you made (which improves readability, and more importantly, eases 
extensibility when adding support for more elements). 

BTW, you might want to double-check if there shouldn't be a trailing '/' here 
(or maybe, Mozilla already/only supports the existing value?)

 const XMLNS = "http://www.w3.org/XML/1998/namespace";
 const XMLNS = "http://www.w3.org/XML/1998/namespace/";

    
    

    
  
Well, the first gives a reference page on the W3C site, whereas the second gives 
a 404-Not-Found, so I assume the first is correct ;-) That's sicking getting 
these things right, as usual...

Gerv

    
    

    
  
You are right.

[The confusion is with this other one:
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsNameSpaceManager.cpp#
32

 32 static const char kXMLNSNameSpaceURI[] = "http://www.w3.org/2000/xmlns/";
]

    
    

    
  
Note that although W3C's usually do, there is nothing that says that namespace
URIs should resolve.

    
    

    
  
Netscape and RSS anyone? but we aren't validating anything so ...

    
    

    
  
This _will_ get in. :-)

Gerv
Target Milestone: --- → mozilla0.9.1

    
    

    
  
ok, this looks good at first pass, but can I have a diff -bw, just for my own
sanity?

    
    

    
  


  

    
    

    
  
awesome! sr=alecf

    
    

    
  
Checked in. Wahey!

Gerv
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED

    
    

    
  
cc'ing L10n. UI changes after UI freeze
Product: Browser → Seamonkey

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: