<!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">
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>
<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:
'z4006-CodeFileBrowser-flexible-case-and-base-PhilBellalouna-2019Dec11-23h00m-pb.1.cs.st'
asFileEntry) base: (CodeFile newFromFile:
'4006-CodeFileBrowser-flexible-case-and-base-PhilBellalouna-2019Dec11-23h00m-pb.1.cs.st'
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>
<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>
<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>
<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>
<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>
<br>
Thanks for doing this!<br>
<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>
<br>
<br>
On 1/6/2020 2:31 AM, Phil B wrote:
<blockquote
cite="mid:CAMJMOeiqiQPA9bn69U-PxotqVarSLGVQWoOQCW0JC7C1gxo=AA@mail.gmail.com"
type="cite">
<div dir="ltr"><bump></div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Thu, Dec 12, 2019 at 2:27
AM Phil B <<a moz-do-not-send="true"
href="mailto:pbpublist@gmail.com">pbpublist@gmail.com</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 dir="ltr">OK, I think this version addresses the items
from the previous version[1] and your feedback. There were
about half a dozen methods with breaking changes (i.e.
either needed to be redefined or renamed) to align
CodeFileBrowser->SystemDictionary and
PseudoClass->ClassDescription but I didn't see any
obvious users of them other than CodeFileBrowser so I don't
think this impacts anything else.
<div><br>
</div>
<div>[1] The one exception was item 2... this can't be
'fixed' without doing a rather convoluted 3-way diff with
the image which may, or may not, make sense depending on
what you're looking at. So for now at least, I'm leaving
it as is.</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Thu, Dec 5, 2019 at
2:59 PM Phil B <<a moz-do-not-send="true"
href="mailto:pbpublist@gmail.com" target="_blank">pbpublist@gmail.com</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 dir="auto">
<div>Juan,<br>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Thu, Dec 5,
2019, 9:57 AM Juan Vuletich <<a
moz-do-not-send="true"
href="mailto:juan@jvuletich.org" target="_blank">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>
I really like what you are doing here.<br>
</div>
</blockquote>
</div>
</div>
<div dir="auto"><br>
</div>
<div dir="auto">Thanks... it's taken a couple attempts
to get something that didn't turn into a mess</div>
<div dir="auto"><br>
</div>
<div dir="auto">
<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>
However there is one detail I think can be
improved. You call the stuff that can be
compared 'target' and 'source'. The problem is
that 'source' means 'source code' in many places
and this could be confusing. Besides, 'target'
doesn't obviously mean 'the reference we are
comparing against'. DifferenceFinder calls them
'base' and 'case'. This sounds clearer to me. Or
may be there are better names to be found.<br>
</div>
</blockquote>
</div>
</div>
<div dir="auto"><br>
</div>
<div dir="auto">I wasn't wild about the naming either.
So baseCodeSource and caseCodeSource? (I'm not crazy
about the CodeSource part of the name either but
wanted to come up with a convention that would hint at
how future implementations/interfaces might be named)</div>
<div dir="auto"><br>
</div>
<div dir="auto">Also, are you OK with the conditional
logic on the menu items? I wanted to check on this
before doing something similar with the button bar
(i.e. when an image isn't the target/base (or is it
case?) most of the items should probably be disabled)</div>
<div dir="auto"><br>
</div>
<div dir="auto">
<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>
Thanks,<br>
<pre cols="72">--
Juan Vuletich
<a moz-do-not-send="true" href="http://www.cuis-smalltalk.org" rel="noreferrer" target="_blank">www.cuis-smalltalk.org</a>
<a moz-do-not-send="true" href="https://github.com/Cuis-Smalltalk/Cuis-Smalltalk-Dev" rel="noreferrer" target="_blank">https://github.com/Cuis-Smalltalk/Cuis-Smalltalk-Dev</a>
<a moz-do-not-send="true" href="https://github.com/jvuletich" rel="noreferrer" target="_blank">https://github.com/jvuletich</a>
<a moz-do-not-send="true" href="https://www.linkedin.com/in/juan-vuletich-75611b3" rel="noreferrer" target="_blank">https://www.linkedin.com/in/juan-vuletich-75611b3</a>
@JuanVuletich</pre>
</div>
</blockquote>
</div>
</div>
<div dir="auto"><br>
</div>
<div dir="auto">Thanks,</div>
<div dir="auto">Phil</div>
<div dir="auto"><br>
</div>
<div dir="auto">
<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>
<br>
On 12/5/2019 6:34 AM, Phil B wrote:
<blockquote type="cite">
<div dir="ltr">I finally had some time to work
on this and got something working. I wanted
to throw it out there for feedback before
going any further with it as there are a few
outstanding issues.
<div><br>
</div>
<div>What this changeset should allow for is
any pairing of Smalltalk
image/changeset/package in a
CodeFileBrowserWindow (though most of the
changes are actually in CodeFileBrowser)
By default, you can still open a browser
window from a FileListWindow as always.
But now when you want a non-standard
browser you can 'CodeFileBrowser
browseCodeSource: (CodePackageFile
newFromFile: someFileEntry) target:
(CodePackageFile newFromFile:
someOtherFileEntry)' (you can also either
leave one of the arguments as nil or pass
Smalltalk to use the image as your
source/target... see 3 below) You should
also be able to add arbitrary other code
sources (i.e. remote images as
sources/targets are a possibility) as
well.</div>
<div><br>
</div>
<div>Here are a few notes on where things
stand:</div>
<div><br>
</div>
<div>1) One of the things that I ran into
doing this that I really liked was
disabling the 'false color' diff
formatting of the browser so I made this
an optional flag. This is handy when
you're looking at a piece of code and want
to see missing methods/classes in the
image. It's far from perfect (i.e. it is
very dumb as methods defined in the source
aren't taken into account), but I still
find it handy and if it makes sense this
could be made a UI toggle.</div>
<div><br>
</div>
<div>2) Related to 1 is that the existing
diffing code doesn't know what to do in
non-standard configurations (i.e. when
target is not the Smalltalk image) so view
as source always appears to be running
with 'false color' off (this is a bug)</div>
<div><br>
</div>
<div>3) Currently the image can only be a
target (work in progress)</div>
<div><br>
</div>
<div>4) There was one breaking method
change:
PseudoClass>>includesSelector: so if
I've missed any senders that depend on the
old behavior (I only saw one obvious one)
they will need minor changes.</div>
<div><br>
</div>
<div>5) Pane menus are a bit more complex
since some of the options don't make sense
if either the image isn't the target or a
changeset/package isn't the source.</div>
<div><br>
</div>
<div>6) Related to 5, the button pane needs
conditional work (still thinking about how
to best deal with this)</div>
<div><br>
</div>
<div>So what do you think? Anything obvious
I missed?</div>
<div><br>
</div>
<div>Phil</div>
<div><br>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Sat,
Nov 2, 2019 at 2:50 PM Phil B <<a
moz-do-not-send="true"
href="mailto:pbpublist@gmail.com"
rel="noreferrer" target="_blank">pbpublist@gmail.com</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 dir="ltr">
<div dir="ltr">Juan,</div>
<div><br>
</div>
OK, that's basically what I was
thinking. I just wanted to see if you
were open to it first since it will
require some rework (I'm not currently
thinking a rewrite is in order... we'll
see) of the existing model classes.
I'll start with the model and if the
direction makes sense then can enhance
the views to expose the functionality.
<div>
<div>
<div><br>
</div>
<div>Thanks,</div>
<div>Phil</div>
<div><br>
<div class="gmail_quote">
<div dir="ltr"
class="gmail_attr">On Sat, Nov
2, 2019 at 1:35 PM Juan
Vuletich <<a
moz-do-not-send="true"
href="mailto:juan@jvuletich.org"
rel="noreferrer"
target="_blank">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;">On
10/29/2019 2:17 PM, Phil B via
Cuis-dev wrote:<br>
> I was looking into adding
the ability to easily
analyze/diff two <br>
> sources of Smalltalk code
(i.e. basically the same image
vs. changeset <br>
> or image vs. package
functionality we currently
have but expanding the <br>
> capability to allow for
any two arbitrary sources
whether changesets <br>
> vs. changeset, package
vs. package, package vs.
changeset or some <br>
> other pairing entirely)
and it seems a bit harder than
it needs to be <br>
> since the existing
implementation assumes you
will always be comparing <br>
> the image vs. something
else.<br>
><br>
> If this already exists
and I'm looking in the wrong
place, pointers <br>
> are appreciated. If it
doesn't, would there be any
objection to <br>
> modifying/cleaning up
this code so that we could
optionally set an <br>
> 'other' Code*Browser that
would be used rather than
always assuming <br>
> the image? A related
enhancement could be to make
the Code*Browser <br>
> code a bit smarter so
that we don't need to
explicitly determine if <br>
> 'this is a package' or
'this is a changeset' unless
you want to. <br>
> Basically I'm asking if
we would be willing to trade a
(slight) <br>
> increase in
implementation complexity for
increased code source <br>
> flexibility here?<br>
<br>
Hi Phil,<br>
<br>
What you want is not yet done.
I think it is a good idea. I
suggest <br>
writing new (or improving
existing) Model classes, for
code already <br>
loaded in the image (in base
image or in packages), and for
code in <br>
files (CodePackageFile, etc).
Ideally, the same View classes
(i.e. the <br>
Morphic Windows) should be the
same and work on whatever
model. Models <br>
should be polymorphic enough
so that comparing works
irrespectively of <br>
the models involved.<br>
<br>
This could be a nice
improvement for the system.<br>
<br>
Cheers,<br>
<br>
-- <br>
Juan Vuletich<br>
<a moz-do-not-send="true"
href="http://www.cuis-smalltalk.org"
rel="noreferrer noreferrer"
target="_blank">www.cuis-smalltalk.org</a><br>
<a moz-do-not-send="true"
href="https://github.com/Cuis-Smalltalk/Cuis-Smalltalk-Dev"
rel="noreferrer noreferrer"
target="_blank">https://github.com/Cuis-Smalltalk/Cuis-Smalltalk-Dev</a><br>
<a moz-do-not-send="true"
href="https://github.com/jvuletich"
rel="noreferrer noreferrer"
target="_blank">https://github.com/jvuletich</a><br>
<a moz-do-not-send="true"
href="https://www.linkedin.com/in/juan-vuletich-75611b3"
rel="noreferrer noreferrer"
target="_blank">https://www.linkedin.com/in/juan-vuletich-75611b3</a><br>
@JuanVuletich<br>
<br>
</blockquote>
</div>
</div>
</div>
</div>
</div>
</blockquote>
</div>
</blockquote>
<br>
</div>
</blockquote>
</div>
</div>
</div>
</blockquote>
</div>
</blockquote>
</div>
</blockquote>
<br>
<br>
</body>
</html>