Smalltalk › Squeak › Squeak - Dev

Fix for MergeBrowser

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

11 messages Options

Karl Ramberg

Fix for MergeBrowser

I'm pretty sure this is a proper fix so the MCMergeBrowser can open a browser from the context menu:

MCMethodDefinition>>packageInfo

^ self workingCopy! !

Best,

Karl

MCMethodDefinition.kfr.1.cs (274 bytes) Download Attachment

Bert Freudenberg

Re: Fix for MergeBrowser

On 24.05.2016, at 17:17, karl ramberg <[hidden email]> wrote:
>
> I'm pretty sure this is a proper fix so the MCMergeBrowser can open a browser from the context menu:

This appears to work fine for me even without that change.

> MCMethodDefinition>>packageInfo
>
> ^ self workingCopy! !

… and a WorkingCopy most definitely is not the same as a PackageInfo ;)

- Bert -

smime.p7s (5K) Download Attachment

Karl Ramberg

Re: Fix for MergeBrowser

I know packageInfo is not the same as workingCopy but I'm not sure how to reference it from the merge browser.

How can this be working in your image ? packageInfo is not implemented for MCMethodDefinition...

It's called from

MCWorkingCopyBrowser>>mcModelFor: aClassOrMethodReference

^ aClassOrMethodReference mcModel ifNil:

[ | pkgName rep | (UIManager confirm: 'Okay to add historical repository ' , (rep := MCHttpRepository trunkBackup) description , ' to ' , (pkgName := aClassOrMethodReference packageInfo packageName) , '?') ifTrue:

[ (MCWorkingCopy forPackageNamed: pkgName) repositoryGroup addRepository: rep.

aClassOrMethodReference mcModel ] ]

Best,

Karl

On Tue, May 24, 2016 at 5:56 PM, Bert Freudenberg <[hidden email]> wrote:

On 24.05.2016, at 17:17, karl ramberg <[hidden email]> wrote:
>
> I'm pretty sure this is a proper fix so the MCMergeBrowser can open a browser from the context menu:

This appears to work fine for me even without that change.

> MCMethodDefinition>>packageInfo
>
> ^ self workingCopy! !

… and a WorkingCopy most definitely is not the same as a PackageInfo ;)

- Bert -

Bert Freudenberg

Re: Fix for MergeBrowser

Which menu item are you trying? You wrote “can open a browser from the context menu” and for me both cmd-b and the context menu work for opening a System Browser.

- Bert -

On 24.05.2016, at 18:27, karl ramberg <[hidden email]> wrote:

I know packageInfo is not the same as workingCopy but I'm not sure how to reference it from the merge browser.

How can this be working in your image ? packageInfo is not implemented for MCMethodDefinition...

It's called from
MCWorkingCopyBrowser>>mcModelFor: aClassOrMethodReference
^ aClassOrMethodReference mcModel ifNil:
[ | pkgName rep | (UIManager confirm: 'Okay to add historical repository ' , (rep := MCHttpRepository trunkBackup) description , ' to ' , (pkgName := aClassOrMethodReference packageInfo packageName) , '?') ifTrue:
[ (MCWorkingCopy forPackageNamed: pkgName) repositoryGroup addRepository: rep.
aClassOrMethodReference mcModel ] ]

Best,
Karl

On Tue, May 24, 2016 at 5:56 PM, Bert Freudenberg <[hidden email]> wrote:
On 24.05.2016, at 17:17, karl ramberg <[hidden email]> wrote:
>
> I'm pretty sure this is a proper fix so the MCMergeBrowser can open a browser from the context menu:

This appears to work fine for me even without that change.

> MCMethodDefinition>>packageInfo
>
> ^ self workingCopy! !

… and a WorkingCopy most definitely is not the same as a PackageInfo ;)

- Bert -

smime.p7s (5K) Download Attachment

Karl Ramberg

Re: Fix for MergeBrowser

Hm, weird.

I removed the fix I did and now it works...

Not sure what was going on

Best,

Karl

On Tue, May 24, 2016 at 6:33 PM, Bert Freudenberg <[hidden email]> wrote:

Which menu item are you trying? You wrote “can open a browser from the context menu” and for me both cmd-b and the context menu work for opening a System Browser.

- Bert -

On 24.05.2016, at 18:27, karl ramberg <[hidden email]> wrote:

I know packageInfo is not the same as workingCopy but I'm not sure how to reference it from the merge browser.

How can this be working in your image ? packageInfo is not implemented for MCMethodDefinition...

It's called from
MCWorkingCopyBrowser>>mcModelFor: aClassOrMethodReference
^ aClassOrMethodReference mcModel ifNil:
[ | pkgName rep | (UIManager confirm: 'Okay to add historical repository ' , (rep := MCHttpRepository trunkBackup) description , ' to ' , (pkgName := aClassOrMethodReference packageInfo packageName) , '?') ifTrue:
[ (MCWorkingCopy forPackageNamed: pkgName) repositoryGroup addRepository: rep.
aClassOrMethodReference mcModel ] ]

Best,
Karl

On Tue, May 24, 2016 at 5:56 PM, Bert Freudenberg <[hidden email]> wrote:
On 24.05.2016, at 17:17, karl ramberg <[hidden email]> wrote:
>
> I'm pretty sure this is a proper fix so the MCMergeBrowser can open a browser from the context menu:

This appears to work fine for me even without that change.

> MCMethodDefinition>>packageInfo
>
> ^ self workingCopy! !

… and a WorkingCopy most definitely is not the same as a PackageInfo ;)

- Bert -

Karl Ramberg

Re: Fix for MergeBrowser

Ok, it only fails if MCMethodDefinition>>mcModel is nil in MCWorkingCopyBrowser>>mcModelFor: aClassOrMethodReference

Once you have the MCMethodDefinition>>mcModel installed it will work the next time, so my fix is still necessary

Best,

Karl

On Tue, May 24, 2016 at 6:41 PM, karl ramberg <[hidden email]> wrote:

Hm, weird.
I removed the fix I did and now it works...

Not sure what was going on

Best,
Karl

On Tue, May 24, 2016 at 6:33 PM, Bert Freudenberg <[hidden email]> wrote:
Which menu item are you trying? You wrote “can open a browser from the context menu” and for me both cmd-b and the context menu work for opening a System Browser.

- Bert -

On 24.05.2016, at 18:27, karl ramberg <[hidden email]> wrote:

I know packageInfo is not the same as workingCopy but I'm not sure how to reference it from the merge browser.

How can this be working in your image ? packageInfo is not implemented for MCMethodDefinition...

It's called from
MCWorkingCopyBrowser>>mcModelFor: aClassOrMethodReference
^ aClassOrMethodReference mcModel ifNil:
[ | pkgName rep | (UIManager confirm: 'Okay to add historical repository ' , (rep := MCHttpRepository trunkBackup) description , ' to ' , (pkgName := aClassOrMethodReference packageInfo packageName) , '?') ifTrue:
[ (MCWorkingCopy forPackageNamed: pkgName) repositoryGroup addRepository: rep.
aClassOrMethodReference mcModel ] ]

Best,
Karl

On Tue, May 24, 2016 at 5:56 PM, Bert Freudenberg <[hidden email]> wrote:
On 24.05.2016, at 17:17, karl ramberg <[hidden email]> wrote:
>
> I'm pretty sure this is a proper fix so the MCMergeBrowser can open a browser from the context menu:

This appears to work fine for me even without that change.

> MCMethodDefinition>>packageInfo
>
> ^ self workingCopy! !

… and a WorkingCopy most definitely is not the same as a PackageInfo ;)

- Bert -

Bert Freudenberg

Re: Fix for MergeBrowser

Okay, but the workaround should be

MCMethodDefinition>>packageInfo

^self asMethodReference packageInfo

and the *real* fix would get the packageInfo from the package version you’re trying to merge (because the method definition in question might not even be in the system).

- Bert -

On 24.05.2016, at 19:31, karl ramberg <[hidden email]> wrote:

Ok, it only fails if MCMethodDefinition>>mcModel is nil in MCWorkingCopyBrowser>>mcModelFor: aClassOrMethodReference

Once you have the MCMethodDefinition>>mcModel installed it will work the next time, so my fix is still necessary

Best,
Karl

On Tue, May 24, 2016 at 6:41 PM, karl ramberg <[hidden email]> wrote:
Hm, weird.
I removed the fix I did and now it works...

Not sure what was going on

Best,
Karl

On Tue, May 24, 2016 at 6:33 PM, Bert Freudenberg <[hidden email]> wrote:
Which menu item are you trying? You wrote “can open a browser from the context menu” and for me both cmd-b and the context menu work for opening a System Browser.

- Bert -

On 24.05.2016, at 18:27, karl ramberg <[hidden email]> wrote:

I know packageInfo is not the same as workingCopy but I'm not sure how to reference it from the merge browser.

How can this be working in your image ? packageInfo is not implemented for MCMethodDefinition...

It's called from
MCWorkingCopyBrowser>>mcModelFor: aClassOrMethodReference
^ aClassOrMethodReference mcModel ifNil:
[ | pkgName rep | (UIManager confirm: 'Okay to add historical repository ' , (rep := MCHttpRepository trunkBackup) description , ' to ' , (pkgName := aClassOrMethodReference packageInfo packageName) , '?') ifTrue:
[ (MCWorkingCopy forPackageNamed: pkgName) repositoryGroup addRepository: rep.
aClassOrMethodReference mcModel ] ]

Best,
Karl

On Tue, May 24, 2016 at 5:56 PM, Bert Freudenberg <[hidden email]> wrote:
On 24.05.2016, at 17:17, karl ramberg <[hidden email]> wrote:
>
> I'm pretty sure this is a proper fix so the MCMergeBrowser can open a browser from the context menu:

This appears to work fine for me even without that change.

> MCMethodDefinition>>packageInfo
>
> ^ self workingCopy! !

… and a WorkingCopy most definitely is not the same as a PackageInfo ;)

- Bert -

smime.p7s (5K) Download Attachment

Chris Muller-3

Re: Fix for MergeBrowser

In reply to this post by Karl Ramberg

Hi Karl, mcModel is for the history function, the code shouldn't be
getting there for a regular browse class. Are you sure you didn't
select "browse mc history" or "browse mc origin"?

PS -- I still plan to try to fix the mc history function again..

On Tue, May 24, 2016 at 12:31 PM, karl ramberg <[hidden email]> wrote:

> Ok, it only fails if MCMethodDefinition>>mcModel is nil in
> MCWorkingCopyBrowser>>mcModelFor: aClassOrMethodReference
>
> Once you have the MCMethodDefinition>>mcModel installed it will work the
> next time, so my fix is still necessary
>
>
>
> Best,
> Karl
>
> On Tue, May 24, 2016 at 6:41 PM, karl ramberg <[hidden email]> wrote:
>>
>> Hm, weird.
>> I removed the fix I did and now it works...
>>
>> Not sure what was going on
>>
>> Best,
>> Karl
>>
>> On Tue, May 24, 2016 at 6:33 PM, Bert Freudenberg <[hidden email]>
>> wrote:
>>>
>>> Which menu item are you trying? You wrote “can open a browser from the
>>> context menu” and for me both cmd-b and the context menu work for opening a
>>> System Browser.
>>>
>>> - Bert -
>>>
>>>
>>> On 24.05.2016, at 18:27, karl ramberg <[hidden email]> wrote:
>>>
>>>
>>> I know packageInfo is not the same as workingCopy but I'm not sure how to
>>> reference it from the merge browser.
>>>
>>> How can this be working in your image ? packageInfo is not implemented
>>> for MCMethodDefinition...
>>>
>>>
>>> It's called from
>>> MCWorkingCopyBrowser>>mcModelFor: aClassOrMethodReference
>>> ^ aClassOrMethodReference mcModel ifNil:
>>> [ | pkgName rep | (UIManager confirm: 'Okay to add historical repository
>>> ' , (rep := MCHttpRepository trunkBackup) description , ' to ' , (pkgName :=
>>> aClassOrMethodReference packageInfo packageName) , '?') ifTrue:
>>> [ (MCWorkingCopy forPackageNamed: pkgName) repositoryGroup addRepository:
>>> rep.
>>> aClassOrMethodReference mcModel ] ]
>>>
>>> Best,
>>> Karl
>>>
>>> On Tue, May 24, 2016 at 5:56 PM, Bert Freudenberg <[hidden email]>
>>> wrote:
>>>>
>>>> On 24.05.2016, at 17:17, karl ramberg <[hidden email]> wrote:
>>>> >
>>>> > I'm pretty sure this is a proper fix so the MCMergeBrowser can open a
>>>> > browser from the context menu:
>>>>
>>>> This appears to work fine for me even without that change.
>>>>
>>>> > MCMethodDefinition>>packageInfo
>>>> >
>>>> > ^ self workingCopy! !
>>>>
>>>> … and a WorkingCopy most definitely is not the same as a PackageInfo ;)
>>>>
>>>> - Bert -
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>
>
>
>

Karl Ramberg

Re: Fix for MergeBrowser

Thanks, Chris and Bert.

I think I found the reason I had problems.

If you change instance variables but not any methods you get the methodListMenu: from MCOperationsBrowser but not the one from MCCodeTool.

There is only 'browse origins' in the MCOperationsBrowser>>methodListMenu:

So the fix must be to fix MCCodeTool>>methodListMenu: to at least give option to 'browse full' so one can get to the class.

Best,

Karl

On Tue, May 24, 2016 at 9:27 PM, Chris Muller <[hidden email]> wrote:

Hi Karl, mcModel is for the history function, the code shouldn't be
getting there for a regular browse class. Are you sure you didn't
select "browse mc history" or "browse mc origin"?

PS -- I still plan to try to fix the mc history function again..

On Tue, May 24, 2016 at 12:31 PM, karl ramberg <[hidden email]> wrote:
> Ok, it only fails if MCMethodDefinition>>mcModel is nil in
> MCWorkingCopyBrowser>>mcModelFor: aClassOrMethodReference
>
> Once you have the MCMethodDefinition>>mcModel installed it will work the
> next time, so my fix is still necessary
>
>
>
> Best,
> Karl
>
> On Tue, May 24, 2016 at 6:41 PM, karl ramberg <[hidden email]> wrote:
>>
>> Hm, weird.
>> I removed the fix I did and now it works...
>>
>> Not sure what was going on
>>
>> Best,
>> Karl
>>
>> On Tue, May 24, 2016 at 6:33 PM, Bert Freudenberg <[hidden email]>
>> wrote:
>>>
>>> Which menu item are you trying? You wrote “can open a browser from the
>>> context menu” and for me both cmd-b and the context menu work for opening a
>>> System Browser.
>>>
>>> - Bert -
>>>
>>>
>>> On 24.05.2016, at 18:27, karl ramberg <[hidden email]> wrote:
>>>
>>>
>>> I know packageInfo is not the same as workingCopy but I'm not sure how to
>>> reference it from the merge browser.
>>>
>>> How can this be working in your image ? packageInfo is not implemented
>>> for MCMethodDefinition...
>>>
>>>
>>> It's called from
>>> MCWorkingCopyBrowser>>mcModelFor: aClassOrMethodReference
>>> ^ aClassOrMethodReference mcModel ifNil:
>>> [ | pkgName rep | (UIManager confirm: 'Okay to add historical repository
>>> ' , (rep := MCHttpRepository trunkBackup) description , ' to ' , (pkgName :=
>>> aClassOrMethodReference packageInfo packageName) , '?') ifTrue:
>>> [ (MCWorkingCopy forPackageNamed: pkgName) repositoryGroup addRepository:
>>> rep.
>>> aClassOrMethodReference mcModel ] ]
>>>
>>> Best,
>>> Karl
>>>
>>> On Tue, May 24, 2016 at 5:56 PM, Bert Freudenberg <[hidden email]>
>>> wrote:
>>>>
>>>> On 24.05.2016, at 17:17, karl ramberg <[hidden email]> wrote:
>>>> >
>>>> > I'm pretty sure this is a proper fix so the MCMergeBrowser can open a
>>>> > browser from the context menu:
>>>>
>>>> This appears to work fine for me even without that change.
>>>>
>>>> > MCMethodDefinition>>packageInfo
>>>> >
>>>> > ^ self workingCopy! !
>>>>
>>>> … and a WorkingCopy most definitely is not the same as a PackageInfo ;)
>>>>
>>>> - Bert -
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>
>
>
>

Karl Ramberg

Re: Fix for MergeBrowser

I can't publish. Image give me strange crash...

Anyway here is the code

On Wed, May 25, 2016 at 10:55 AM, karl ramberg <[hidden email]> wrote:

Thanks, Chris and Bert.
I think I found the reason I had problems.
If you change instance variables but not any methods you get the methodListMenu: from MCOperationsBrowser but not the one from MCCodeTool.

There is only 'browse origins' in the MCOperationsBrowser>>methodListMenu:

So the fix must be to fix MCCodeTool>>methodListMenu: to at least give option to 'browse full' so one can get to the class.

Best,
Karl

On Tue, May 24, 2016 at 9:27 PM, Chris Muller <[hidden email]> wrote:
Hi Karl, mcModel is for the history function, the code shouldn't be
getting there for a regular browse class. Are you sure you didn't
select "browse mc history" or "browse mc origin"?

PS -- I still plan to try to fix the mc history function again..

On Tue, May 24, 2016 at 12:31 PM, karl ramberg <[hidden email]> wrote:
> Ok, it only fails if MCMethodDefinition>>mcModel is nil in
> MCWorkingCopyBrowser>>mcModelFor: aClassOrMethodReference
>
> Once you have the MCMethodDefinition>>mcModel installed it will work the
> next time, so my fix is still necessary
>
>
>
> Best,
> Karl
>
> On Tue, May 24, 2016 at 6:41 PM, karl ramberg <[hidden email]> wrote:
>>
>> Hm, weird.
>> I removed the fix I did and now it works...
>>
>> Not sure what was going on
>>
>> Best,
>> Karl
>>
>> On Tue, May 24, 2016 at 6:33 PM, Bert Freudenberg <[hidden email]>
>> wrote:
>>>
>>> Which menu item are you trying? You wrote “can open a browser from the
>>> context menu” and for me both cmd-b and the context menu work for opening a
>>> System Browser.
>>>
>>> - Bert -
>>>
>>>
>>> On 24.05.2016, at 18:27, karl ramberg <[hidden email]> wrote:
>>>
>>>
>>> I know packageInfo is not the same as workingCopy but I'm not sure how to
>>> reference it from the merge browser.
>>>
>>> How can this be working in your image ? packageInfo is not implemented
>>> for MCMethodDefinition...
>>>
>>>
>>> It's called from
>>> MCWorkingCopyBrowser>>mcModelFor: aClassOrMethodReference
>>> ^ aClassOrMethodReference mcModel ifNil:
>>> [ | pkgName rep | (UIManager confirm: 'Okay to add historical repository
>>> ' , (rep := MCHttpRepository trunkBackup) description , ' to ' , (pkgName :=
>>> aClassOrMethodReference packageInfo packageName) , '?') ifTrue:
>>> [ (MCWorkingCopy forPackageNamed: pkgName) repositoryGroup addRepository:
>>> rep.
>>> aClassOrMethodReference mcModel ] ]
>>>
>>> Best,
>>> Karl
>>>
>>> On Tue, May 24, 2016 at 5:56 PM, Bert Freudenberg <[hidden email]>
>>> wrote:
>>>>
>>>> On 24.05.2016, at 17:17, karl ramberg <[hidden email]> wrote:
>>>> >
>>>> > I'm pretty sure this is a proper fix so the MCMergeBrowser can open a
>>>> > browser from the context menu:
>>>>
>>>> This appears to work fine for me even without that change.
>>>>
>>>> > MCMethodDefinition>>packageInfo
>>>> >
>>>> > ^ self workingCopy! !
>>>>
>>>> … and a WorkingCopy most definitely is not the same as a PackageInfo ;)
>>>>
>>>> - Bert -
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>
>
>
>

MCCodeTool.kfr.1.cs (2K) Download Attachment

marcel.taeumel

Re: Fix for MergeBrowser

Hi Karl,

after a quick look in the DNU I always get when "browse origin" of a method change, I see that MCWorkingCopyBrowser class >> #mcModelFor: seems to be not prepared to process an mc definition. Yes, MCDefinition does implement #mcModel but the code in #mcModelFor: suggests that the author expected classes or method references only. :-) So, the call in MCOperationsBrowser >> #browseSelectorOrigin seems to misuse #mcModelFor: at the moment.

Anyway, I wonder why MCWorkingCopyBrowser class >> #mcModelFor: does exist at all because it bypasses the idea of polymorphism, which actually is employed in #mcModel being implemented in Class, MethodReference, MCHttpRepository, and MCRepository.

My suggestion for anybody who is willing to fix this is to move the code from #mcModelFor: into those implementors of #mcModel, who need it. Otherwise, it seems *really* difficult to understand this additional dispatch -- and when to use it -- in the future for other developers.

Best,
Marcel