<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
<title></title>
</head>
<body bgcolor="#ffffff" text="#000000">
Phil,<br>
<br>
On 1/8/2020 12:59 PM, Phil B wrote:
<blockquote
cite="mid:CAMJMOehV=uUwMqQ7+k2-0jgO-TLRN40vjDGqVdzfgN12ZzjHGA@mail.gmail.com"
type="cite">
<div dir="ltr">
<div dir="ltr">Juan,</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Wed, Jan 8, 2020 at 9:30
AM Juan Vuletich <<a moz-do-not-send="true"
href="mailto:juan@jvuletich.org">juan@jvuletich.org</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin: 0px 0px 0px
0.8ex; border-left: 1px solid rgb(204, 204, 204);
padding-left: 1ex;">
<div bgcolor="#ffffff"> Hi Phil,<br>
<br>
Apologies for the delay in reviewing this.<br>
<br>
I took some time to review the code and play with it.
Thanks for using the 'case' and 'base' names. Code is now
easier to follow.<br>
</div>
</blockquote>
<div><br>
</div>
<div>Cool.</div>
<div> </div>
<blockquote class="gmail_quote" style="margin: 0px 0px 0px
0.8ex; border-left: 1px solid rgb(204, 204, 204);
padding-left: 1ex;">
<div bgcolor="#ffffff"> <br>
I used your very change set and a slightly edited version
as guinea pigs, and compared them with:<br>
<br>
CodeFileBrowser browseCodeSource: (CodeFile newFromFile: '<a
moz-do-not-send="true"
href="http://z4006-CodeFileBrowser-flexible-case-and-base-PhilBellalouna-2019Dec11-23h00m-pb.1.cs.st"
target="_blank">z4006-CodeFileBrowser-flexible-case-and-base-PhilBellalouna-2019Dec11-23h00m-pb.1.cs.st</a>'
asFileEntry) base: (CodeFile newFromFile: '<a
moz-do-not-send="true"
href="http://4006-CodeFileBrowser-flexible-case-and-base-PhilBellalouna-2019Dec11-23h00m-pb.1.cs.st"
target="_blank">4006-CodeFileBrowser-flexible-case-and-base-PhilBellalouna-2019Dec11-23h00m-pb.1.cs.st</a>'
asFileEntry)<br>
<br>
There's a detail I'd change, though. I agree with you that
'new' methods (in case but not in base) should be color
highlighed by default. But I don't think we need the new
toggle. Just make it as if #shouldShowFalseColorDiffs
always answers false. If the user doesn't want diffing,
they can always click on [show...] and select '[o]
source'.<br>
</div>
</blockquote>
<div><br>
</div>
<div>There are two aspects to this flag: first, I was trying
to match existing behavior wherever possible and it made
sense. That way if people complained after it got
integrated, it was more likely to be due to a bug rather
than a deliberate change. </div>
</div>
</div>
</blockquote>
<br>
We all appreciate this attitude.<br>
<br>
<blockquote
cite="mid:CAMJMOehV=uUwMqQ7+k2-0jgO-TLRN40vjDGqVdzfgN12ZzjHGA@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div>But the other was a question that I never really fully
resolved: do we want to try to maintain the old half-differ
behavior that the image currently has? That's really what
this flag is doing... are you saying you don't want this old
behavior anymore? </div>
</div>
</div>
</blockquote>
<br>
Yes. That's what I said. Diffing was done originally in regular
browsers. Support in CodeFileBrowser has always been half baked.
This is a good opportunity to improve it.<br>
<br>
<blockquote
cite="mid:CAMJMOehV=uUwMqQ7+k2-0jgO-TLRN40vjDGqVdzfgN12ZzjHGA@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div>If not, why don't we go with a half step of turning it
off by default and see how people react before removing the
option entirely?</div>
</div>
</div>
</blockquote>
<br>
Because current behavior is inconsistent, and keeping it adds
unneeded complexity to the code. Let's make it better and simpler at
the same time.<br>
<br>
<blockquote
cite="mid:CAMJMOehV=uUwMqQ7+k2-0jgO-TLRN40vjDGqVdzfgN12ZzjHGA@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin: 0px 0px 0px
0.8ex; border-left: 1px solid rgb(204, 204, 204);
padding-left: 1ex;">
<div bgcolor="#ffffff"> <br>
Besides, in #infoViewContents, when method is not in
'base' it says '**NEW** Method not in the system'. It
should read something like '**ADDED** Method not in base'.
Same for class. It says 'Class not in the system', and
should say 'Class not in base'. All these messages should
stay like now if base is Smalltalk image, though.<br>
</div>
</blockquote>
<div><br>
</div>
<div>So change 'system' to 'base' if base is not an image?
(keep in mind that it is possible that base could be a file
and case could be the image or base and case could both be
images. Also, there's no guarantee that 'image' refers to
the currently, locally running image...)</div>
</div>
</div>
</blockquote>
<br>
Maybe we could have a couple new ivars: 'caseDescription' and
'baseDescription', and build the user messages with them. This would
make it easier to tweak them until we are happy.<br>
<br>
<blockquote
cite="mid:CAMJMOehV=uUwMqQ7+k2-0jgO-TLRN40vjDGqVdzfgN12ZzjHGA@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin: 0px 0px 0px
0.8ex; border-left: 1px solid rgb(204, 204, 204);
padding-left: 1ex;">
<div bgcolor="#ffffff"> <br>
Another issue is that in '*** removed methods ***' it
shows actively methods deleted in a change set, but not
missing methods (defined in 'base' but not in 'case').
Maybe, when 'base' is not the Smalltalk image, a new
category with 'methods not included' or such could be
added.<br>
</div>
</blockquote>
<div><br>
</div>
<div>The current behavior doesn't show this does it? I was
trying to minimize breakage or behavior changes (at least
for now), but we can change this if you want. This is an
area I had planned to fill out more later once it seemed
like the core changes are solid.</div>
</div>
</div>
</blockquote>
<br>
Current behavior doesn't do it because it was thought for
ChangeSets. A ChangeSet doesn't include "all the stuff", just the
part it affects. Therefore, deletions must be explicit to be
acknowledged. A packages is different. And this becomes clearer when
base and case can be different versions of a package, possibly not
loaded in the image.<br>
<br>
But we can do this after integrating your first work, checking that
people is happy with them, and fixing any breakage.<br>
<blockquote
cite="mid:CAMJMOehV=uUwMqQ7+k2-0jgO-TLRN40vjDGqVdzfgN12ZzjHGA@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin: 0px 0px 0px
0.8ex; border-left: 1px solid rgb(204, 204, 204);
padding-left: 1ex;">
<div bgcolor="#ffffff"> <br>
Finally (I know, I'm asking lots of things!), it would be
cool to have, in the message list, added methods in red,
and removed/missing methods in blue. This might not be
trivial. Not sure if our lists can do color highlighting.<br>
</div>
</blockquote>
<div><br>
</div>
<div>I agree that would be nice to have but I don't think
lists can currently do that. If you add colored
highlighting support to lists, I'll use it :-)</div>
</div>
</div>
</blockquote>
<br>
Fair enough! I'll do it.<br>
<blockquote
cite="mid:CAMJMOehV=uUwMqQ7+k2-0jgO-TLRN40vjDGqVdzfgN12ZzjHGA@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin: 0px 0px 0px
0.8ex; border-left: 1px solid rgb(204, 204, 204);
padding-left: 1ex;">
<div bgcolor="#ffffff"> <br>
And maybe we could change the colors... Red for new stuff
is too strong, and usually means 'NO' (like a stop sign).
Maybe Green would be better. Not sure.<br>
</div>
</blockquote>
<div><br>
</div>
<div>I agree... red for add makes no sense. But that was
consistent with current behavior so I kept it. I would
prefer something green (or blue) for adds, yellow for
changes and red for deletes. However, I wanted to hold off
on changing coloring logic until after we're sure that these
changes otherwise make sense and don't break anything.
While I'm reasonably comfortable with the changes relative
to the base image, I was hoping to get it integrated to see
if this breaks anyone else's stuff before actively trying to
change the look/behavior too much to minimize confusion.</div>
</div>
</div>
</blockquote>
<br>
Ok. This makes sense. Let's change colors later. Hopefully soon,
before this loses moment.<br>
<br>
<blockquote
cite="mid:CAMJMOehV=uUwMqQ7+k2-0jgO-TLRN40vjDGqVdzfgN12ZzjHGA@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<blockquote class="gmail_quote" style="margin: 0px 0px 0px
0.8ex; border-left: 1px solid rgb(204, 204, 204);
padding-left: 1ex;">
<div bgcolor="#ffffff"> <br>
Thanks for doing this!<br>
<br>
Cheers,<br>
<pre cols="72">--
Juan Vuletich
<a moz-do-not-send="true" href="http://www.cuis-smalltalk.org" target="_blank">www.cuis-smalltalk.org</a>
<a moz-do-not-send="true" href="https://github.com/Cuis-Smalltalk/Cuis-Smalltalk-Dev" target="_blank">https://github.com/Cuis-Smalltalk/Cuis-Smalltalk-Dev</a>
<a moz-do-not-send="true" href="https://github.com/jvuletich" target="_blank">https://github.com/jvuletich</a>
<a moz-do-not-send="true" href="https://www.linkedin.com/in/juan-vuletich-75611b3" target="_blank">https://www.linkedin.com/in/juan-vuletich-75611b3</a>
@JuanVuletich</pre>
<br>
</div>
</blockquote>
<div><br>
</div>
<div>Thanks,</div>
<div>Phil</div>
</div>
</div>
</blockquote>
<br>
Cheers,<br>
<pre class="moz-signature" cols="72">--
Juan Vuletich
<a class="moz-txt-link-abbreviated" href="http://www.cuis-smalltalk.org">www.cuis-smalltalk.org</a>
<a class="moz-txt-link-freetext" href="https://github.com/Cuis-Smalltalk/Cuis-Smalltalk-Dev">https://github.com/Cuis-Smalltalk/Cuis-Smalltalk-Dev</a>
<a class="moz-txt-link-freetext" href="https://github.com/jvuletich">https://github.com/jvuletich</a>
<a class="moz-txt-link-freetext" href="https://www.linkedin.com/in/juan-vuletich-75611b3">https://www.linkedin.com/in/juan-vuletich-75611b3</a>
@JuanVuletich</pre>
</body>
</html>