[Cuis-dev] Browser class hierarchy & functionality

Juan Vuletich juan at jvuletich.org
Thu Jan 9 04:21:31 PST 2020 

Phil,

On 1/8/2020 12:59 PM, Phil B wrote:
> Juan,
>
> On Wed, Jan 8, 2020 at 9:30 AM Juan Vuletich <juan at jvuletich.org 
> <mailto: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
>     <http://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
>     <http://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.

We all appreciate this attitude.

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

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.

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

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.

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

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.

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

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.

But we can do this after integrating your first work, checking that 
people is happy with them, and fixing any breakage.
>
>
>     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 :-)

Fair enough! I'll do 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.

Ok. This makes sense. Let's change colors later. Hopefully soon, before 
this loses moment.

>
>
>     Thanks for doing this!
>
>     Cheers,
>
>     -- 
>     Juan Vuletich
>     www.cuis-smalltalk.org  <http://www.cuis-smalltalk.org>
>     https://github.com/Cuis-Smalltalk/Cuis-Smalltalk-Dev
>     https://github.com/jvuletich
>     https://www.linkedin.com/in/juan-vuletich-75611b3
>     @JuanVuletich
>
>
>
> Thanks,
> Phil

Cheers,

-- 
Juan Vuletich
www.cuis-smalltalk.org
https://github.com/Cuis-Smalltalk/Cuis-Smalltalk-Dev
https://github.com/jvuletich
https://www.linkedin.com/in/juan-vuletich-75611b3
@JuanVuletich

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cuis.st/mailman/archives/cuis-dev/attachments/20200109/ed5a208f/attachment.htm>

More information about the Cuis-dev mailing list