195285 - [junk] - Junk mail icon default placement in Thread pane should be to the right of Subject

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[esther]

Per new spec, default placement of the Junk Mail icon should be on the right
side of default placement of the Subject.   

1. Launch app
2. Add new profile
3. Add mail account and enable Junk Mail Controls

Expected, JMC column should be visible and should be the column to the right of
Subject.

Comment 1

[(not reading, please use [e-mail] instead)]

fixed, as part of bug #195640.

Comment 2

[esther]

Using trunk build 20030312 on winxp and linux I tested this with a new profile
POP account, new profile IMAP account, and Migrated 4.7 mail account.  All did
the right thing with the placement of the Trash column in Thread pane.  I tested
this with an existing profile that had the Trash column in a place other than
our default and it stayed in the same place as previously placed and did not
move to the default location.  This is correct behavior and is verified

Comment 3

[esther]

Verified for macosx too. 

Comment 4

[esther]

Need to reopen for case I had not tested that is broken. Using trunk build
20030318 on winxp, mac osx and linux an existing 7.01 profile places the new
Junk colunmn after the Priority column.  The existing profile I tested above was
with an exising 1.3 profile that already had a Junk column.

Comment 5

[(not reading, please use [e-mail] instead)]

accepting, I'll look into this issue.

Comment 6

[Peter Lairo]

Is putting a narrow collumn in the middle between two wide collumns such a good
idea? Me thinks a better place would be to the *left* of the subject collumn.
Easily discovered, and falsely marked spam not easily overlooked. INVALID?

Comment 7

[Peter Lairo]

Attachment: Screenshot of junk collumn on LEFT of Subject collumn.

PS. Why is the "Priority" (default:on) more important than the "Size" collumn
(default:off)?

Comment 8

[(not reading, please use [e-mail] instead)]

over to shuehan

Comment 9

[(not reading, please use [e-mail] instead)]

peter, the problem we found with your suggestion is that the column interferes
with the thread column and the thread twisty.

users can always move the column, using dnd.

Comment 10

[[e-mail]]

Attachment: Non-working patch :-(

The idea of this patch is that instead of collecting new columns at the end
they are inserted logically after the physically previous column. Unfortunately
it crashes :-(

Comment 11

[shliang]

Attachment: patch

place junk column to the immediate right of subject column for upgrading
existing profile

Comment 12

[(not reading, please use [e-mail] instead)]

Comment on attachment 121920 [details] [diff] [review]
patch

colsToIncrement makes me nervous, because I don't know what js will do here.

if I have:

colsToIncrement[0] = foo;
colsToIncrement[10000] = bar;

will that allocate a giant colsToIncrement array?

I worry, because oridinals can be huge, right?

or is js smart enough to not allocate a giant array of nulls?

also, what if one of the ordinals is MAX (lxr for
ordinal="2147483647")

also, do we have to worry about splitters?

maybe first, we should normalize?
lxr for _ensureColumnOrder()

let's see what neil thinks.  he was attempting to fix this in a completely
different way.

Comment 13

[shliang]

Attachment: patch

Comment 14

[(not reading, please use [e-mail] instead)]

how have you solved the splitter issue that we discussed in your cube today?

Comment 15

[(not reading, please use [e-mail] instead)]

Comment on attachment 122149 [details] [diff] [review]
patch

clearing review request, until the splitter issue is resolved.

maybe the way to do that is to call threadTree._ensureColumnOrder();
again, after you set the ordinal for the junkCol?

Comment 16

[shliang]

the splitters stay in the right places because ensure column order is first 
called on the tree, then only the tree columns are moved while the splitters 
stay in place.

Comment 17

[(not reading, please use [e-mail] instead)]

Comment on attachment 122149 [details] [diff] [review]
patch

r/sr=sspitzer, a=sspitzer for 1.4 beta.

thanks for fixing this, shuehan.

Comment 18

[jag (Peter Annema)]

Comment on attachment 122149 [details] [diff] [review]
patch

I don't think you need threadTree._ensureColumnOrder there. If you do need it,
we need to provide access to it from <tree> in a non-private way.

However, I think we should really have something like |tree.moveColumn(0, 5)|
or |tree.moveColumn(myCol, beforeCol, true)| and |tree.moveColumn(myCol,
afterCol, false)|.

There's already a method on tree that does this, and makes sure the ordinals
stay consistent, we can just reuse that (with a slightly different name).

Comment 19

[jag (Peter Annema)]

I filed bug 204142 to deal with making tree support what we want to do.

Your code should now become something like:

function UpgradeThreadPaneUI()
{
  try {
    var threadPaneUIVersion = pref.getIntPref("mailnews.ui.threadpane.version");

    if (threadPaneUIVersion < 3) {
      var subjectCol = document.getElementById("subjectCol");
      var junkCol = document.getElementById("junkStatusCol");
      var threadTree = document.getElementById("threadTree");

      threadTree.moveColumn(junkCol, subjectCol.boxObject.nextSibling);
      // or if we go with the simple patch:
      // var beforeCol = subjectCol.boxObject.nextSibling;
      // if (beforeCol)
      //   threadTree.moveColumn(junkCol, beforeCol, true);
      // else // subjectCol was the last column, put it after
      //   threadTree.moveColumn(junkCol, subjectCol, false);

      if (threadPaneUIVersion == 1) {
        var labelCol = document.getElementById("labelCol");
        labelCol.setAttribute("hidden", "true");
      }

      pref.setIntPref("mailnews.ui.threadpane.version", 3);
    }
  } catch (ex) {
    dump("UpgradeThreadPane: ex = " + ex + "\n");
  }
}

In writing this I guess it would be nicer if we could do
tree.moveColumn(junkCol, subjectCol, false); // move junkCol after subjectCol

That would require a little fixing of _reorderColumn which currently assumes
that if |aBefore| is false, |colBefore| is the last column.

Comment 20

[jag (Peter Annema)]

If you want to get this in right now you could even use the code from the
comment with _reorderColumn instead of moveColumn:

      var beforeCol = subjectCol.boxObject.nextSibling;
      if (beforeCol)
        threadTree._reorderColumn(junkCol, beforeCol, true);
      else // subjectCol was the last column, put it after
        threadTree._reorderColumn(junkCol, subjectCol, false);

If you do, add a comment to bug 204142 to fix this code once it is fixed.

Note to self: figure out how ordinals are getting updated for hidden columns.

Comment 21

[jag (Peter Annema)]

Actually, that should be (in all code snippets using this particular approach):

  var beforeCol = subjectCol.boxObject.nextSibling.boxObject.nextSibling;

We want to skip the splitter.

Comment 22

[shliang]

Attachment: patch

Comment 23

[jag (Peter Annema)]

Comment on attachment 122493 [details] [diff] [review]
patch

r=jag

Comment 24

[(not reading, please use [e-mail] instead)]

Comment on attachment 122493 [details] [diff] [review]
patch

jag is ok with us doing threadTree._reorderColumn?

part of the reason he didn't like _ensureColumns() was it was a "private"
method on tree.

Comment 25

[(not reading, please use [e-mail] instead)]

Comment on attachment 122493 [details] [diff] [review]
patch

sr/a=sspitzer

note, this will effect all those users of prior releases who might have moved
the junk column around.

for example, 1.4 a users.
(I think 1.3 final, too)

Comment 26

[shliang]

resolving

Comment 27

[esther]

Using trunk build 20030508 on winxp, macosx and linux and a 7.01 and/or 7.02
profile this is fixed.  Junk column is in the right place. Verified.