Comment 1 Markus Duft 2012-01-17 05:14:09 EST

proposed fix at http://egit.eclipse.org/r/#change,4967

Comment 2 Dani Megert 2012-01-17 12:13:08 EST

> i think there is a syncExec missing in the propertyChange handler in the
> GitHistoryPage...

Nope, this is not needed. The wrong merge for bug 368526 causes the IAE.

Comment 3 Robin Rosenberg 2012-01-17 17:41:32 EST

(In reply to comment #2)
> > i think there is a syncExec missing in the propertyChange handler in the
> > GitHistoryPage...
> 
> Nope, this is not needed. The wrong merge for bug 368526 causes the IAE.

That code should survive being called at any time, with a change in the
setting or not, so a (a)sycnexec is needed for that reason, if not for
your particular call sequence.

Comment 4 Robin Rosenberg 2012-01-17 18:34:51 EST

Why is a change in the relative date setting, published?

Comment 5 Dani Megert 2012-01-18 03:09:36 EST

(In reply to comment #4)
> Why is a change in the relative date setting, published?

It is not, that's the point. Here's how the old code looked like:

	if (UIPreferences.RESOURCEHISTORY_SHOW_RELATIVE_DATE.equals(event
			.getProperty()))
		if (graph.setRelativeDate(((Boolean) event.getNewValue())
					.booleanValue()))
			graph.getTableView().refresh();

Yes, not easy readable without braces and with bad indentation. Basically, the refresh only happened if the event was for RESOURCEHISTORY_SHOW_RELATIVE_DATE property and only if the value changed.

You changed the code to *always* call refresh() no matter what the property is:

	if (UIPreferences.RESOURCEHISTORY_SHOW_RELATIVE_DATE.equals(event
			.getProperty()))
		graph.setRelativeDate(isShowingRelativeDates());
		graph.getTableView().refresh();


The latest suggested patch at least only updates when the property matches but it sill updates when the value remains the same. Also, the relative date setting is a UI property that gets changed via UI preference (in the UI thread). Hence that code always worked in the past. Adding a (a)syncExec is not needed.

The correct code should be as indicated in my very first patch:

	if (UIPreferences.RESOURCEHISTORY_SHOW_RELATIVE_DATE.equals(event
			.getProperty()))
		if (graph.setRelativeDate(isShowingRelativeDates()))
			graph.getTableView().refresh();
	}

Comment 6 Markus Duft 2012-01-18 03:29:38 EST

phew. is it absolutely unthinkable that such properties will be changed in the future from a background thread? i think of some "preference import job" or something like that... i'd still sleep better if an [a]syncExec would be there. at least a big fat // WARNING: should be there i think...

Comment 7 Dani Megert 2012-01-18 03:30:10 EST

> if (graph.setRelativeDate(isShowingRelativeDates()))

OK, I see that this no longer works since the signature of setRelativeDate(...) got changed. I'll upload a new patch.

Comment 8 Dani Megert 2012-01-18 03:44:52 EST

(In reply to comment #6)
> phew. is it absolutely unthinkable that such properties will be changed in the
> future from a background thread? i think of some "preference import job" or
> something like that... i'd still sleep better if an [a]syncExec would be there.
> at least a big fat // WARNING: should be there i think...

It's a bit a gray area but the general rule is that those *calling* UI code (which a property change listener is, since it comes from JFace) are responsible to only call it in the UI thread. That's why e.g. the preference import happens in the UI thread and not in a job. On the other hand, if one knows/expects that the code is called from any thread then an (a)syncExec makes sense.

I generally try to avoid polluting the code with (a)syncExec if possible. This is for two reasons:
1) It can cause deadlocks.
2) It can change the event flow/order and cause ugly things which are hard to debug.

Comment 9 Robin Rosenberg 2012-01-18 12:32:51 EST

(In reply to comment #5)

> The correct code should be as indicated in my very first patch:
> 
>     if (UIPreferences.RESOURCEHISTORY_SHOW_RELATIVE_DATE.equals(event
>             .getProperty()))
>         if (graph.setRelativeDate(isShowingRelativeDates()))
>             graph.getTableView().refresh();
>     }

Aaaaahh.. You're absolutely right.

Comment 10 Dani Megert 2012-01-19 03:45:01 EST

The change got merged.

Comment 11 Dani Megert 2012-01-19 03:45:17 EST

Verified in 1.3.0.201201181724.

Comment 12 Kevin Sawicki 2012-01-26 14:44:14 EST

*** Bug 369418 has been marked as a duplicate of this bug. ***