Bug 337550

Comment 1 Dani Megert 2011-02-24 05:34:46 EST

Looking at the latest patch in 76386 I could even live with the new filter as provided there, if the labels are brought into a consistent state: currently one field has "contains", one "containing" and one has no addition. I would use "contains" for all three fields.

Comment 2 Olexiy Buyanskyy 2011-04-01 09:01:34 EDT

Created attachment 192354 [details]
moved piece of changes from bug 76386

Remember this patch requires the latest patch from bug 76386

Comment 3 Tomasz Zarna 2011-04-13 11:25:09 EDT

(In reply to comment #1)
> I would use "contains" for all three fields.

Makes sense to me, but for the Author field "matches" is more appropriate as this is how this criteria works. Or we could change the way it works ;)

Comment 4 Tomasz Zarna 2011-04-13 11:25:57 EDT

Created attachment 193164 [details]
Olexiy's patch with minor changes

Comment 5 Tomasz Zarna 2011-04-13 11:25:59 EDT

Created attachment 193165 [details]
mylyn/context/zip

Comment 6 Tomasz Zarna 2011-04-15 05:15:28 EDT

I found one minor thing, I hope it's the last one: CVSUIMessages.HistoryFilterDialog_branchname should be renamed to HistoryFilterDialog_branchName to be consistent with other names. 

Olexiy, could you please update the latest patch? This way we could safely say it's your contribution.

Dani, should I open a new bug for comment 0, or are we gonna keep this one open?

Comment 7 Dani Megert 2011-04-15 06:06:47 EDT

(In reply to comment #6)
> I found one minor thing, I hope it's the last one:
> CVSUIMessages.HistoryFilterDialog_branchname should be renamed to
> HistoryFilterDialog_branchName to be consistent with other names. 
> 
> Olexiy, could you please update the latest patch? This way we could safely say
> it's your contribution.
> 
> Dani, should I open a new bug for comment 0, or are we gonna keep this one
> open?

I think we can leave it as is since the idea is to add a pattern. What I'd like to see though is F1 help that explains which patterns are supported.

Comment 8 Tomasz Zarna 2011-04-15 09:36:12 EDT

(In reply to comment #7)
> What I'd like to see though is F1 help that explains which patterns are supported.

Filed bug 342969 for updating the doc. The F1 help for the filter dialog is just one sentence, I guess you meant the "Filtering CVS Revisions in the History View" page in Help.

Comment 9 Dani Megert 2011-04-18 02:34:27 EDT

(In reply to comment #8)
> (In reply to comment #7)
> > What I'd like to see though is F1 help that explains which patterns are supported.
> 
> Filed bug 342969 for updating the doc. The F1 help for the filter dialog is
> just one sentence, I guess you meant the "Filtering CVS Revisions in the
> History View" page in Help.

The whole F1 help for that dialog needs work:
1. There is a (?) button but that doesn't work (opens tool tip instead of tray)
2. The F1 text does not link to a help page which explains the dialog's options,
   see e.g. Open Type's F1 help.

Comment 10 Szymon Brandys 2011-04-26 06:11:58 EDT

Tomek, I would leave the 'branch' filter as is in the latest patch and update Help early during RC1. If there are any improvements planned, like these from comment 0, please raise a separate bug for it and set for 3.8.

Comment 11 Olexiy Buyanskyy 2011-04-26 08:32:14 EDT

Created attachment 194046 [details]
rename branchname to branchName

(In reply to comment #6)
> I found one minor thing, I hope it's the last one:
> CVSUIMessages.HistoryFilterDialog_branchname should be renamed to
> HistoryFilterDialog_branchName to be consistent with other names.
> 
> Olexiy, could you please update the latest patch? This way we could safely say
> it's your contribution.
Done

Comment 12 Tomasz Zarna 2011-05-02 07:52:11 EDT

(In reply to comment #10)
> Tomek, I would leave the 'branch' filter as is in the latest patch 

Fixed, but I have used none of the patches provided.

> and update Help early during RC1. 

Already scheduled for RC1, see bug 342969.

> If there are any improvements planned, like these from
> comment 0, please raise a separate bug for it and set for 3.8.

Filed bug 344438.

Comment 13 Dani Megert 2011-05-02 08:07:20 EDT

Tomek, as we are now in the end game (http://www.eclipse.org/eclipse/development/plans/freeze_plan_3_7.php) there are certain rules that must be followed, e.g. for RC1 you need a second committer approving that the bug is worth fixing plus you need a review+ on the patch from a reviewer (approver and reviewer can but need not to be the same person). Of course both of that must happen before a fix is committed to HEAD ;-).

Personally, I'd say this is feature work which should not be done during RC* but if Szymon approves it for RC1 and reviews the patch then that's fine with me.

Comment 14 Tomasz Zarna 2011-05-02 08:46:55 EDT

(In reply to comment #13)
> Tomek, as we are now in the end game

Sorry, forgot about that. I just got back from vacation and thought it's a straightforward fix.

Comment 15 Tomasz Zarna 2011-05-04 04:55:01 EDT

Created attachment 194682 [details]
Applied patch

Comment 16 Szymon Brandys 2011-05-04 05:22:03 EDT

Looks good. Go ahead.

Comment 17 Tomasz Zarna 2011-05-04 07:35:31 EDT

Thanks Sim. Already committed to HEAD at the time of comment 12. Available in builds >=I20110504-0800.