Expose methods to insert/remove indices in the m_keys array in msgDBView

RESOLVED FIXED in Thunderbird 23.0

Status

MailNews Core
Backend
--
enhancement
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: alta88, Assigned: alta88)

Tracking

unspecified
Thunderbird 23.0
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

5 years ago
the ability to create new views in the message list would be greatly enhanced.
(Assignee)

Updated

5 years ago
Assignee: nobody → alta88
(Assignee)

Comment 1

5 years ago
Created attachment 696078 [details] [diff] [review]
patch.


david, pls advise if you're not the correct reviewer.
Attachment #696078 - Flags: review?(mozilla)
(Assignee)

Updated

5 years ago
Attachment #696078 - Flags: review?(mozilla)
Attachment #696078 - Flags: review?(mbanner)
Attachment #696078 - Flags: feedback?(mozilla)

Comment 2

4 years ago
Comment on attachment 696078 [details] [diff] [review]
patch.

feedback+ for exposing some way of doing this. Maybe you can explain what you want to do with this so we can better understand which options(s) below to go with.

Why would you want to insert multiple instances of the same key, flags, and level? Either you should insert just one row or pass in an array of msgKeys, msgFlags, and levels, or just expose/use InsertEmptyRows. Also, you should point out in the idl that these methods don't do the notifications that they need to do (i.e.,  NoteChange(mIndicesToNoteChange[i], +/-numRows, nsMsgViewNotificationCode::insertOrDelete);

and that the caller will need to do the notifications or else the tree will get out of sync.

or else, make the methods do the notifications (though that will cause an invalidate + repaint, so you need to have the right data in place)
Attachment #696078 - Flags: feedback?(mozilla) → feedback+
(Assignee)

Comment 3

4 years ago
(In reply to David :Bienvenu from comment #2)
> Comment on attachment 696078 [details] [diff] [review]
> patch.
> 
> feedback+ for exposing some way of doing this. Maybe you can explain what
> you want to do with this so we can better understand which options(s) below
> to go with.
> 
> Why would you want to insert multiple instances of the same key, flags, and
> level? Either you should insert just one row or pass in an array of msgKeys,
> msgFlags, and levels, or just expose/use InsertEmptyRows.

thanks for the feedback.  the goal is to allow more space (# of rows, a small arbitrary number) in the view for each individual message in the db.  so in fact it's useful to insert 3 rows with the exact key/flag/level of the 'real' row.  but it also doesn't preclude adding single rows with any values.


> Also, you should
> point out in the idl that these methods don't do the notifications that they
> need to do (i.e.,  NoteChange(mIndicesToNoteChange[i], +/-numRows,
> nsMsgViewNotificationCode::insertOrDelete);
> 
> and that the caller will need to do the notifications or else the tree will
> get out of sync.
> 
> or else, make the methods do the notifications (though that will cause an
> invalidate + repaint, so you need to have the right data in place)

i tend to think the caller should ensure all this, using rowCountChanged() and invalidate() for repainting the view, as the methods to do what NoteChange basically does are already available.  if the insertion/removal methods were to automatically notify, it would preclude finer control: one may want to do a number of insertions before calling rowCountChanged() or otherwise handle batching.  but perhaps NoteChanged() should be exposed too?

so the 2 methods as exposed only concern themselves with the actual insertion/removal in the array(s) and sanity checking.  as for data, it would be driven by tree methods as usual, once the rows are there.

(you can probably think of harder issues like sorting/grouping, but so far it looks reasonable.)
(Assignee)

Comment 4

4 years ago
note: a new patch will be forthcoming to enable insertion/removal into m_folders, in the case of search/xfv views..

Comment 5

4 years ago
(In reply to alta88 from comment #3)

> > Why would you want to insert multiple instances of the same key, flags, and
> > level? Either you should insert just one row or pass in an array of msgKeys,
> > msgFlags, and levels, or just expose/use InsertEmptyRows.
> 
> thanks for the feedback.  the goal is to allow more space (# of rows, a
> small arbitrary number) in the view for each individual message in the db. 
> so in fact it's useful to insert 3 rows with the exact key/flag/level of the
> 'real' row.  but it also doesn't preclude adding single rows with any values.
so, you could use InsertEmptyRows for the multiple row case where you're going to fill in the real values yourself, and a method that inserted an already filled in singleton row for the other case? i.e., change your new method to just take a single key/flag/value but no row count, and expose InsertEmptyRows in the idl. That seems a bit cleaner than using a single method that sometimes gets valid values and sometimes not.
> 
> 
> > Also, you should
> > point out in the idl that these methods don't do the notifications that they
> > need to do (i.e.,  NoteChange(mIndicesToNoteChange[i], +/-numRows,
> > nsMsgViewNotificationCode::insertOrDelete);
> > 
> > and that the caller will need to do the notifications or else the tree will
> > get out of sync.
> > 
> > or else, make the methods do the notifications (though that will cause an
> > invalidate + repaint, so you need to have the right data in place)
> 
> i tend to think the caller should ensure all this, using rowCountChanged()
> and invalidate() for repainting the view, as the methods to do what
> NoteChange basically does are already available.  if the insertion/removal
> methods were to automatically notify, it would preclude finer control: one
> may want to do a number of insertions before calling rowCountChanged() or
> otherwise handle batching.  but perhaps NoteChanged() should be exposed too?

I'd much rather have the callers use NoteChanged than doing their own rowCountChanged() and invalidation.
(Assignee)

Comment 6

4 years ago
(In reply to David :Bienvenu from comment #5)
> (In reply to alta88 from comment #3)
> 
> > > Why would you want to insert multiple instances of the same key, flags, and
> > > level? Either you should insert just one row or pass in an array of msgKeys,
> > > msgFlags, and levels, or just expose/use InsertEmptyRows.
> > 
> > thanks for the feedback.  the goal is to allow more space (# of rows, a
> > small arbitrary number) in the view for each individual message in the db. 
> > so in fact it's useful to insert 3 rows with the exact key/flag/level of the
> > 'real' row.  but it also doesn't preclude adding single rows with any values.
> so, you could use InsertEmptyRows for the multiple row case where you're
> going to fill in the real values yourself, and a method that inserted an
> already filled in singleton row for the other case? i.e., change your new
> method to just take a single key/flag/value but no row count, and expose
> InsertEmptyRows in the idl. That seems a bit cleaner than using a single
> method that sometimes gets valid values and sometimes not.
> > 

if there is a row at x, i want to insert 3 rows at x+1, each to have the msgKey etc of row x.  using InsertEmptyRows() would mean going back and updating the arrays; using a single insert method would mean a 3x js loop rather than InsertElementsAt() on the c++ array.  no?  InsertEmptyRows() can be exposed, but without filling the rows in later, and before rowCountChanged(), it doesn't leave a useful tree.  so any caller would need to fill it in anyway, may as well be one shot.  anyway, i'll do it however you advise, just would like to understand it first.

> > 
> > > Also, you should
> > > point out in the idl that these methods don't do the notifications that they
> > > need to do (i.e.,  NoteChange(mIndicesToNoteChange[i], +/-numRows,
> > > nsMsgViewNotificationCode::insertOrDelete);
> > > 
> > > and that the caller will need to do the notifications or else the tree will
> > > get out of sync.
> > > 
> > > or else, make the methods do the notifications (though that will cause an
> > > invalidate + repaint, so you need to have the right data in place)
> > 
> > i tend to think the caller should ensure all this, using rowCountChanged()
> > and invalidate() for repainting the view, as the methods to do what
> > NoteChange basically does are already available.  if the insertion/removal
> > methods were to automatically notify, it would preclude finer control: one
> > may want to do a number of insertions before calling rowCountChanged() or
> > otherwise handle batching.  but perhaps NoteChanged() should be exposed too?
> 
> I'd much rather have the callers use NoteChanged than doing their own
> rowCountChanged() and invalidation.

sure.  should i assume you mean exposing it and documenting it should be used post insert, or do you mean adding it inline in an insert method?
(Assignee)

Comment 7

4 years ago
Created attachment 705368 [details] [diff] [review]
patch


1. expose NoteChange.
2. support for search/xfvf view indices, check for m_folders.
Attachment #696078 - Attachment is obsolete: true
Attachment #696078 - Flags: review?(mbanner)
Attachment #705368 - Flags: review?(mozilla)
(Assignee)

Comment 8

4 years ago
david, could we finish this off?
(Assignee)

Comment 9

4 years ago
Created attachment 729026 [details] [diff] [review]
patch


tweak for 0 index.

it was suggested to me that an alternative reviewer might be better, so back to mbanner.
Attachment #705368 - Attachment is obsolete: true
Attachment #705368 - Flags: review?(mozilla)
Attachment #729026 - Flags: review?(mbanner)
Comment on attachment 729026 [details] [diff] [review]
patch

Review of attachment 729026 [details] [diff] [review]:
-----------------------------------------------------------------

The general idea is ok, but we need some small improvements to the interfaces. It'd be great if you could do a unit test as well, so that we don't break the functions accidentally in future - especially as they are not used by default. I think you should be able to base something around mailnews/base/test/unit/test_nsMsgDBView.js.

::: mailnews/base/public/nsIMsgDBView.idl
@@ +405,5 @@
>  
>    /**
> +   * Insert rows into the view. The caller should use NoteChange() below to
> +   * update the view.
> +   * 

nit: unnecessary whitespace at end of line (several places in this file).

@@ +413,5 @@
> +   * @param aFlags   msgFlags.
> +   * @param aLevel   treeview indent level.
> +   * @param aFolder  nsIMsgFolder, required for search/xfvf views.
> +   *
> +   * @return         true if success, false otherwise.

Why not just use xpcom success/fail values/exceptions as is standard and is what you are doing in removeTreeRows?

@@ +415,5 @@
> +   * @param aFolder  nsIMsgFolder, required for search/xfvf views.
> +   *
> +   * @return         true if success, false otherwise.
> +   */
> +  boolean insertTreeRows(in nsMsgViewIndex aIndex, in long aNumRows,

I think aNumRows should be unsigned, as this would simplify the function checks, and there shouldn't ever be a case of negative values anyway.

@@ +426,5 @@
> +   * 
> +   * @param aIndex   view index for removal start.
> +   * @param aNumRows number of rows to remove.
> +   */
> +  void removeTreeRows(in nsMsgViewIndex aIndex, in long aNumRows);

Again, aNumRows can be unsigned.

@@ +435,5 @@
> +   * @param aFirstLineChanged   first view index for changed rows.
> +   * @param aNumRows            number of rows changed.
> +   * @param aChangeType         changeType.
> +   */
> +  void NoteChange(in nsMsgViewIndex aFirstLineChanged, in long aNumRows,

I think we should take the opportunity and change aNumRows as well here.

::: mailnews/base/src/nsMsgDBView.cpp
@@ +5516,5 @@
>    m_flags.RemoveElementsAt(viewIndex, numRows);
>    m_levels.RemoveElementsAt(viewIndex, numRows);
>  }
>  
> +NS_IMETHODIMP nsMsgDBView::InsertTreeRows(nsMsgViewIndex viewIndex,

nit: These argument names should follow the same names as defined in the idl files, i.e. aViewIndex etc

@@ +5527,5 @@
> +{
> +  NS_ENSURE_ARG(success);
> +  *success = false;
> +
> +  if (viewIndex < 0 || numRows < 0 || GetSize() < int32_t(viewIndex))

viewIndex is actually a uint32_t and GetSize() also returns uint32_t - so you don't need the viewIndex < 0 check here, and if you change numRows as suggested, you can drop that check as well.

@@ +5553,5 @@
> +NS_IMETHODIMP nsMsgDBView::RemoveTreeRows(nsMsgViewIndex viewIndex,
> +                                          int32_t numRows)
> +{
> +  // Prevent a crash if attempting to remove rows which don't exist.
> +  if (viewIndex < 0 || numRows < 0 || GetSize() < int32_t(viewIndex + numRows))

Similar comment with these checks.

@@ +5561,5 @@
> +
> +  nsCOMArray<nsIMsgFolder> *folders = GetFolders();
> +  if (folders)
> +    // In a search/xfvf view only, remove from m_folders.
> +    folders->RemoveObjectsAt(viewIndex, numRows);

What happens if this fails? Surely we should bubble that up?
Attachment #729026 - Flags: review?(mbanner) → review-
(Assignee)

Comment 11

4 years ago
Created attachment 729174 [details] [diff] [review]
patch


addressed comments.  please confirm it's ok, and then i will submit a r? with test.
Attachment #729026 - Attachment is obsolete: true
Flags: needinfo?(mbanner)
Comment on attachment 729174 [details] [diff] [review]
patch

Review of attachment 729174 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, that's about right, there's a few compilation errors noted below.

::: mailnews/base/src/nsMsgDBView.cpp
@@ +5443,5 @@
>    m_levels.RemoveElementsAt(viewIndex, numRows);
>  }
>  
> +NS_IMETHODIMP nsMsgDBView::InsertTreeRows(nsMsgViewIndex aIndex,
> +                                          int32_t aNumRows,

Should be uint32_t (compile failure)

@@ +5456,5 @@
> +  nsCOMArray<nsIMsgFolder> *folders = GetFolders();
> +  if (folders)
> +  {
> +    // In a search/xfvf view only, a folder is required.
> +    NS_ENSURE_ARG_POINTER(folder);

aFolder I presume.

@@ +5472,5 @@
> +  return NS_ERROR_UNEXPECTED;
> +}
> +
> +NS_IMETHODIMP nsMsgDBView::RemoveTreeRows(nsMsgViewIndex aIndex,
> +                                          int32_t aNumRows)

uint32_t

@@ +6067,5 @@
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP nsMsgDBView::NoteChange(nsMsgViewIndex firstLineChanged,
> +                                      int32_t numChanged,

uint32_t
Attachment #729174 - Flags: feedback+
Flags: needinfo?(mbanner)
(Assignee)

Comment 13

4 years ago
ok, thanks.  should have compiled it myself..

i'll put in a test within the week.
(Assignee)

Comment 14

4 years ago
Created attachment 729539 [details] [diff] [review]
updated patch
Attachment #729174 - Attachment is obsolete: true
(Assignee)

Comment 15

4 years ago
Created attachment 731357 [details] [diff] [review]
patch


it turns out the NoteChange code needs a signed aNumRows to indicate removal..
Attachment #729539 - Attachment is obsolete: true
Attachment #731357 - Flags: review?(mbanner)
(Assignee)

Comment 16

4 years ago
Created attachment 731359 [details] [diff] [review]
test


test, verified on a build.
Attachment #731359 - Flags: review?(mbanner)
Attachment #731357 - Flags: review?(mbanner) → review+
Comment on attachment 731359 [details] [diff] [review]
test

Review of attachment 731359 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/test/unit/test_nsMsgDBView.js
@@ +479,5 @@
>      view_throw("message in tree is " + numMsgInTree + " but should be " +
>               gDBView.numMsgsInView + "\n");
>  }
>  
> +function  test_insert_remove_view_rows() {

nit: only one space before function name
Attachment #731359 - Flags: review?(mbanner) → review+
(Assignee)

Comment 18

4 years ago
Created attachment 735889 [details] [diff] [review]
nit
Attachment #731359 - Attachment is obsolete: true
Attachment #735889 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
This doesn't apply cleanly to comm-central. Can you please post updated patches so I can land them once it reopens? Also, can you please make sure your patches contain all the necessary commit information? Thanks!
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Keywords: checkin-needed
(Assignee)

Comment 20

4 years ago
Created attachment 737133 [details] [diff] [review]
patch+test


updated, thanks.
Attachment #731357 - Attachment is obsolete: true
Attachment #735889 - Attachment is obsolete: true
Attachment #737133 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/c4139faa3d0c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 23.0
(In reply to Mark Banner from comment #10)
> (From update of attachment 729026 [details] [diff] [review])
> > +  nsCOMArray<nsIMsgFolder> *folders = GetFolders();
> > +  if (folders)
> > +    // In a search/xfvf view only, remove from m_folders.
> > +    folders->RemoveObjectsAt(viewIndex, numRows);
> 
> What happens if this fails? Surely we should bubble that up?

This can only fail if viewIndex + numRows is out of range, but we've already checked that, so we could have just called RemoveRows all along (it's overridden by search views).
You need to log in before you can comment on or make changes to this bug.