Last Comment Bug 825012 - Expose methods to insert/remove indices in the m_keys array in msgDBView
: Expose methods to insert/remove indices in the m_keys array in msgDBView
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: Thunderbird 23.0
Assigned To: alta88
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-27 10:51 PST by alta88
Modified: 2013-04-13 16:29 PDT (History)
2 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch. (4.40 KB, patch)
2012-12-27 11:04 PST, alta88
mozilla: feedback+
Details | Diff | Splinter Review
patch (8.43 KB, patch)
2013-01-23 08:07 PST, alta88
no flags Details | Diff | Splinter Review
patch (8.43 KB, patch)
2013-03-25 10:04 PDT, alta88
standard8: review-
Details | Diff | Splinter Review
patch (8.21 KB, patch)
2013-03-25 13:52 PDT, alta88
standard8: feedback+
Details | Diff | Splinter Review
updated patch (8.22 KB, patch)
2013-03-26 07:50 PDT, alta88
no flags Details | Diff | Splinter Review
patch (8.23 KB, patch)
2013-03-29 15:54 PDT, alta88
standard8: review+
Details | Diff | Splinter Review
test (3.02 KB, patch)
2013-03-29 15:57 PDT, alta88
standard8: review+
Details | Diff | Splinter Review
nit (3.02 KB, patch)
2013-04-10 11:23 PDT, alta88
alta88: review+
Details | Diff | Splinter Review
patch+test (11.18 KB, patch)
2013-04-13 07:22 PDT, alta88
alta88: review+
Details | Diff | Splinter Review

Description alta88 2012-12-27 10:51:32 PST
the ability to create new views in the message list would be greatly enhanced.
Comment 1 alta88 2012-12-27 11:04:25 PST
Created attachment 696078 [details] [diff] [review]
patch.


david, pls advise if you're not the correct reviewer.
Comment 2 David :Bienvenu 2013-01-13 08:46:10 PST
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)
Comment 3 alta88 2013-01-14 09:02:12 PST
(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.)
Comment 4 alta88 2013-01-14 11:33:18 PST
note: a new patch will be forthcoming to enable insertion/removal into m_folders, in the case of search/xfv views..
Comment 5 David :Bienvenu 2013-01-15 14:12:20 PST
(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.
Comment 6 alta88 2013-01-15 16:54:06 PST
(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?
Comment 7 alta88 2013-01-23 08:07:05 PST
Created attachment 705368 [details] [diff] [review]
patch


1. expose NoteChange.
2. support for search/xfvf view indices, check for m_folders.
Comment 8 alta88 2013-02-09 10:59:08 PST
david, could we finish this off?
Comment 9 alta88 2013-03-25 10:04:09 PDT
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.
Comment 10 Mark Banner (:standard8) (afk until 26th July) 2013-03-25 10:33:05 PDT
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?
Comment 11 alta88 2013-03-25 13:52:25 PDT
Created attachment 729174 [details] [diff] [review]
patch


addressed comments.  please confirm it's ok, and then i will submit a r? with test.
Comment 12 Mark Banner (:standard8) (afk until 26th July) 2013-03-26 07:24:08 PDT
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
Comment 13 alta88 2013-03-26 07:43:06 PDT
ok, thanks.  should have compiled it myself..

i'll put in a test within the week.
Comment 14 alta88 2013-03-26 07:50:34 PDT
Created attachment 729539 [details] [diff] [review]
updated patch
Comment 15 alta88 2013-03-29 15:54:52 PDT
Created attachment 731357 [details] [diff] [review]
patch


it turns out the NoteChange code needs a signed aNumRows to indicate removal..
Comment 16 alta88 2013-03-29 15:57:15 PDT
Created attachment 731359 [details] [diff] [review]
test


test, verified on a build.
Comment 17 Mark Banner (:standard8) (afk until 26th July) 2013-04-10 10:18:21 PDT
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
Comment 18 alta88 2013-04-10 11:23:37 PDT
Created attachment 735889 [details] [diff] [review]
nit
Comment 19 Ryan VanderMeulen [:RyanVM] 2013-04-12 18:32:27 PDT
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
Comment 20 alta88 2013-04-13 07:22:32 PDT
Created attachment 737133 [details] [diff] [review]
patch+test


updated, thanks.
Comment 21 Ryan VanderMeulen [:RyanVM] 2013-04-13 09:10:07 PDT
https://hg.mozilla.org/comm-central/rev/c4139faa3d0c
Comment 22 neil@parkwaycc.co.uk 2013-04-13 16:29:12 PDT
(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).

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