Bug 250257

Comment 1 Steffen Pingel 2009-11-13 16:44:36 EST

In addition it would be nice if it the displayed columns were configurable, e.g. if an ID column was added that wouldn't be shown by default (bug 295104).

Comment 2 Frank Becker 2009-11-15 13:17:23 EST

Created attachment 152248 [details]
patch

Is this what you need?

Comment 3 Frank Becker 2009-11-15 13:17:28 EST

Created attachment 152249 [details]
mylyn/context/zip

Comment 4 Steffen Pingel 2009-11-16 14:02:25 EST

That looks like a good start. Do you think it would be feasible to make the mechanism generic to potentially work for other tables as well?

We should also consider using a memento to store the table state since this ties in better with the workbench persistence framework and is more robust than serialization.

Comment 5 Frank Becker 2009-11-18 13:56:16 EST

(In reply to comment #4)
> That looks like a good start. Do you think it would be feasible to make the
> mechanism generic to potentially work for other tables as well?
> 
> We should also consider using a memento to store the table state since this
> ties in better with the workbench persistence framework and is more robust than
> serialization.

OK, now I use memento, but I need some time until I have create an generic mechanism.

I try to create an class AbstractTableViewer for this.

Comment 6 Frank Becker 2009-11-19 16:12:00 EST

Created attachment 152640 [details]
generic version

create class AbstractTableViewerConfigurator for the generic part.

TaskEditorAttachmentPart is currently the only place where this is used.

You can also change the column order.

Steffen, can you please review this.

Comment 7 Frank Becker 2009-11-19 16:12:11 EST

Created attachment 152641 [details]
mylyn/context/zip

Comment 8 Frank Becker 2010-02-11 15:51:03 EST

Created attachment 158913 [details]
PreferencePage

Here the new version of the Preferencepage.

Comment 9 Frank Becker 2010-02-11 15:52:09 EST

Created attachment 158914 [details]
AttachmentTable Example

an example of the new AttachmentTable

Comment 10 Frank Becker 2010-02-11 15:54:07 EST

Created attachment 158915 [details]
updated patch

new version of the patch.

Should we do a review?

Comment 11 Frank Becker 2010-02-11 15:54:10 EST

Created attachment 158916 [details]
mylyn/context/zip

Comment 12 Frank Becker 2010-07-11 14:57:44 EDT

Created attachment 173981 [details]
commited patch

Here what I did commit so that I can start with bug# 302652

Please review!

Comment 13 Frank Becker 2010-07-11 14:57:46 EDT

Created attachment 173982 [details]
mylyn/context/zip

Comment 14 Steffen Pingel 2010-07-12 16:56:28 EDT

I noticed a few problems with the latest changes: The attachment table does not have a popup menu any longer and it opens several browser tabs when I double click one attachment. Could you revert the changes for now until we have fixed these regression? Please feel free to commit to a branch if it's too cumbersome to manage the patches. The convention I have used in the past is to prefix the branch name with the bug id, e.g. bug_250257_persists_table_columns and to only branch plug-ins that have changes.

The code design looks fairly good to me. I am concerned though that having AbstractTableViewerConfigurator create the table will make this more difficult to reuse. Do you think we could keep that in the attachment part?

I think we need to take another pass through the UI design and figure out if we can avoid adding a new setting on the preference page. I have seen other tools add a context menu to the table header that allows enabling and disabling of columns. Do you think we could make it work that way?

Comment 15 Frank Becker 2010-07-12 23:45:22 EDT

commited patch is reverted.

Hope that I can find the problem today.

Comment 16 Frank Becker 2010-07-13 13:32:13 EDT

Created attachment 174179 [details]
commited patch

Here my correction so that the context menu is shown up.

I did not include the addMenuListener and not call registerContextMenu

Comment 17 Frank Becker 2010-07-13 13:32:17 EDT

Created attachment 174180 [details]
mylyn/context/zip

Comment 18 Steffen Pingel 2010-07-13 14:33:46 EDT

I still get have the problem that several web browsers get opened up if I double clicking a single attachment. Frank, please revert this patch from head until we have resolved these problems and taken a pass through the architecture. I understand that you want to move forward with this and I will take some time on Thursday to review in more detail but for now I would like to get cvs head back into a better state. We have several people running bootstrapped and we have to ensure that we don't introduce breaking regressions for core functionality such as attachments. I really appreciate your work on this but I think a branch would be better suited to stabilize these changes. Let me know if you need any help with creating a branch.

Comment 19 Frank Becker 2010-07-13 14:45:18 EDT

(In reply to comment #18)
> I still get have the problem that several web browsers get opened up if I double
> clicking a single attachment. Frank, please revert this patch from head until we
> have resolved these problems and taken a pass through the architecture. I
> understand that you want to move forward with this and I will take some time on
> Thursday to review in more detail but for now I would like to get cvs head back
> into a better state. We have several people running bootstrapped and we have to
> ensure that we don't introduce breaking regressions for core functionality such
> as attachments. I really appreciate your work on this but I think a branch would
> be better suited to stabilize these changes. Let me know if you need any help
> with creating a branch.

OK reverted!

Comment 20 Frank Becker 2010-07-13 15:06:56 EDT

Created attachment 174213 [details]
patch

Steffen,

here the corrected version of the patch.

The problem was that I have missed some points when I recreated the patch.

Can you please apply the patch and verify.

Sorry, 

my fault.

Comment 21 Frank Becker 2010-07-13 15:06:59 EDT

Created attachment 174214 [details]
mylyn/context/zip

Comment 22 Frank Becker 2010-07-13 23:47:59 EDT

Created attachment 174251 [details]
patch V6

Sorry last patch was not complete.

Comment 23 Frank Becker 2010-07-13 23:48:02 EDT

Created attachment 174252 [details]
mylyn/context/zip

Comment 24 Steffen Pingel 2010-07-28 20:33:16 EDT

Created attachment 175449 [details]
patch v7

Comment 25 Steffen Pingel 2010-07-28 20:33:19 EDT

Created attachment 175450 [details]
mylyn/context/zip

Comment 26 Steffen Pingel 2010-07-28 20:39:49 EDT

Thanks for all your work on this, Frank! I have generalized the implementation and committed the attached patch. It persists the column width and sort order but there is no UI for configuring visible columns, yet (bug 295104).