[Cuis-dev] Browser class hierarchy & functionality
Juan Vuletich
juan at jvuletich.org
Wed Jan 8 06:30:43 PST 2020
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.
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'.
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.
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.
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.
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.
Thanks for doing this!
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
On 1/6/2020 2:31 AM, Phil B wrote:
> <bump>
>
> On Thu, Dec 12, 2019 at 2:27 AM Phil B <pbpublist at gmail.com
> <mailto:pbpublist at gmail.com>> wrote:
>
> 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.
>
> [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.
>
> On Thu, Dec 5, 2019 at 2:59 PM Phil B <pbpublist at gmail.com
> <mailto:pbpublist at gmail.com>> wrote:
>
> Juan,
>
> On Thu, Dec 5, 2019, 9:57 AM Juan Vuletich <juan at jvuletich.org
> <mailto:juan at jvuletich.org>> wrote:
>
> Hi Phil,
>
> I really like what you are doing here.
>
>
> Thanks... it's taken a couple attempts to get something that
> didn't turn into a mess
>
>
> 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.
>
>
> 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)
>
> 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)
>
>
> Thanks,
>
> --
> 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
>
>
>
> On 12/5/2019 6:34 AM, Phil B wrote:
>> 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.
>>
>> 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.
>>
>> Here are a few notes on where things stand:
>>
>> 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.
>>
>> 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)
>>
>> 3) Currently the image can only be a target (work in
>> progress)
>>
>> 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.
>>
>> 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.
>>
>> 6) Related to 5, the button pane needs conditional work
>> (still thinking about how to best deal with this)
>>
>> So what do you think? Anything obvious I missed?
>>
>> Phil
>>
>>
>> On Sat, Nov 2, 2019 at 2:50 PM Phil B
>> <pbpublist at gmail.com <mailto:pbpublist at gmail.com>> wrote:
>>
>> Juan,
>>
>> 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.
>>
>> Thanks,
>> Phil
>>
>> On Sat, Nov 2, 2019 at 1:35 PM Juan Vuletich
>> <juan at jvuletich.org <mailto:juan at jvuletich.org>> wrote:
>>
>> On 10/29/2019 2:17 PM, Phil B via Cuis-dev wrote:
>> > I was looking into adding the ability to easily
>> analyze/diff two
>> > sources of Smalltalk code (i.e. basically the
>> same image vs. changeset
>> > or image vs. package functionality we currently
>> have but expanding the
>> > capability to allow for any two arbitrary
>> sources whether changesets
>> > vs. changeset, package vs. package, package vs.
>> changeset or some
>> > other pairing entirely) and it seems a bit
>> harder than it needs to be
>> > since the existing implementation assumes you
>> will always be comparing
>> > the image vs. something else.
>> >
>> > If this already exists and I'm looking in the
>> wrong place, pointers
>> > are appreciated. If it doesn't, would there be
>> any objection to
>> > modifying/cleaning up this code so that we
>> could optionally set an
>> > 'other' Code*Browser that would be used rather
>> than always assuming
>> > the image? A related enhancement could be to
>> make the Code*Browser
>> > code a bit smarter so that we don't need to
>> explicitly determine if
>> > 'this is a package' or 'this is a changeset'
>> unless you want to.
>> > Basically I'm asking if we would be willing to
>> trade a (slight)
>> > increase in implementation complexity for
>> increased code source
>> > flexibility here?
>>
>> Hi Phil,
>>
>> What you want is not yet done. I think it is a
>> good idea. I suggest
>> writing new (or improving existing) Model
>> classes, for code already
>> loaded in the image (in base image or in
>> packages), and for code in
>> files (CodePackageFile, etc). Ideally, the same
>> View classes (i.e. the
>> Morphic Windows) should be the same and work on
>> whatever model. Models
>> should be polymorphic enough so that comparing
>> works irrespectively of
>> the models involved.
>>
>> This could be a nice improvement for the system.
>>
>> 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
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cuis.st/mailman/archives/cuis-dev/attachments/20200108/1cd92a55/attachment-0001.htm>
More information about the Cuis-dev
mailing list