[Cuis-dev] Browser class hierarchy & functionality

[Phil B]

Juan,

On Wed, Jan 8, 2020 at 9:30 AM Juan Vuletich <juan at jvuletich.org> wrote:

> Hi Phil,
>
> Apologies for the delay in reviewing this.
>
> 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.
>

Cool.


>
> I used your very change set and a slightly edited version as guinea pigs,
> and compared them with:
>
> 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)
>
> 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'.
>

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.  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?  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?


> 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.
>

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...)


> 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.
>

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.


>
> 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.
>

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 :-)


>
> 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.
>

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.


> Thanks for doing this!
>
> Cheers,
>
> --
> Juan Vuletichwww.cuis-smalltalk.orghttps://github.com/Cuis-Smalltalk/Cuis-Smalltalk-Devhttps://github.com/jvuletichhttps://www.linkedin.com/in/juan-vuletich-75611b3
> @JuanVuletich
>
>
>
Thanks,
Phil
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cuis.st/mailman/archives/cuis-dev/attachments/20200108/85b48421/attachment.htm>