The Inbox: Monticello-nice.734.mcz

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

The Inbox: Monticello-nice.734.mcz

commits-2
A new version of Monticello was added to project The Inbox:
http://source.squeak.org/inbox/Monticello-nice.734.mcz

==================== Summary ====================

Name: Monticello-nice.734
Author: nice
Time: 30 December 2020, 3:57:21.60758 pm
UUID: 863f642a-7795-4eb6-aa6b-76a5af3a8504
Ancestors: Monticello-mt.733

Workaround version history browser bug when 'working copy' is selected.

The snapshot is searched in repositoryGroup, and logically, none is found.
Note that the self selectedInfo isKindOf: MCWorkingCopyAncestry, which does not behave as a regular MCVersionInfo, it does not even answer to versionName...
This somehow accelerate the failure thru an ifError: [] handling, but at the end, the selectedSnapshot is still nil which makes the 'view changes from...' menu fail.

The workaround is not very nice, it use implicit knowledge that 'working copy' will be at top of list (index = 1).

If you can think of nicer fix, please raise voice.

=============== Diff against Monticello-mt.733 ===============

Item was changed:
  ----- Method: MCVersionHistoryBrowser>>selectedSnapshot (in category 'accessing') -----
  selectedSnapshot
+ index = 1 ifTrue: [^package snapshot].
  ^ self snapshotForInfo: self selectedInfo!


Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Monticello-nice.734.mcz

David T. Lewis
On Wed, Dec 30, 2020 at 02:57:23PM +0000, [hidden email] wrote:

> A new version of Monticello was added to project The Inbox:
> http://source.squeak.org/inbox/Monticello-nice.734.mcz
>
> ==================== Summary ====================
>
> Name: Monticello-nice.734
> Author: nice
> Time: 30 December 2020, 3:57:21.60758 pm
> UUID: 863f642a-7795-4eb6-aa6b-76a5af3a8504
> Ancestors: Monticello-mt.733
>
> Workaround version history browser bug when 'working copy' is selected.
>
> The snapshot is searched in repositoryGroup, and logically, none is found.
> Note that the self selectedInfo isKindOf: MCWorkingCopyAncestry, which does not behave as a regular MCVersionInfo, it does not even answer to versionName...
> This somehow accelerate the failure thru an ifError: [] handling, but at the end, the selectedSnapshot is still nil which makes the 'view changes from...' menu fail.
>
> The workaround is not very nice, it use implicit knowledge that 'working copy' will be at top of list (index = 1).
>
> If you can think of nicer fix, please raise voice.
>
Attached is another way to do it without relying on the list index. I have
not tested very much but it seems to be all right.

  MCVersionHistoryBrowser>>snapshotForInfo: aVersionInfo
     "Answer snapshot for aVersionInfo or for the working copy if not found"
     ^ (self repositoryGroup
           versionWithInfo: aVersionInfo
              ifNone: [ package ]) snapshot


Apologies for violating MC package standards by including a method comment ;-)

Dave
 



MCVersionHistoryBrowser-snapshotForInfo.st (373 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Monticello-nice.734.mcz

Nicolas Cellier
Yeah, no comment nowhere and most methods 'as yet unclassified' (we fixed many of those already).
I guess that the idea was that with a clear separation of concerns and limitation of responsibilities, comments coud eventually become moot.
Even classification of methods could be seen as unhelpful if there are few enough per class...
It's been a nice exercise in style.

But in corners like this, nothing is that obvious, sorry but we're well beyond the limit of what an average Smalltalker can follow and understand...
The alternative you're proposing does work thanks to an ugly ifError: handling...
We're sending versionName to a MCWorkingCopyAncestry which is a MNU.
We have absolutely no idea why a MCWorkingCopyAncestry would ever be a good candidate for self selectedInfo...
We have no idea why a MCVersionInfo has to be a specialization of a MCAncestry.
But then, should we subclass MCWorkingCopyAncestry and get a MCWorkingCopyInfo? Inheritance vs composition?
We have no idea why we must search a specific version in MCRepositoryGroup default, rather than the more restricted group registered for this package...
So we end up scanning all the repositories in the default repositoryGroup for getting a snapshot of working copy, which is quiet a vain idea, the snapshot lies in no repository, it's right there under our mouse, in the image.
Clearly not such a good example of crystal clear and straightforward code to my eyes...
I'd rather say crooked!

I don't know when and how exactly we got there...
Is it from inception? Or gradual rot due to quick hacking (we often do the EASIEST thing that could possibly work, rather than the SIMPLEST thing that could possibly sustain)...
But at this stage, we need more than comments. We need deeper clean up.
Every piece of code requires such lifting in order to sustain the burden of age, even the nicest pieces...

Concerning the lack of comments, I have my own opinion: I think it's a great mistake.
The mental image of the zoo of classes and concepts is certainly obvious to the authors, or the experts, once sufficiently fluent.
But the lack of comment or tutorial or even the lightest introduction to the concepts and key implementation choices, will for sure delay the learning curve of newbies.
If you have to deal with such a bug, you're directly thrown into the crooked parts of code, and the austerity of comment is just intimidating. Where to begin the journey in MC?

Let's follow my example: why does each and every MCWorkingCopy has a (nil) versionInfo?
My guess when reading code is that such info could be (have been?) a proxy for rebuilding the ancestry (a cheaper  memory footprint?).
But then what? There no setter but nil... Is it a dead code?
When the bug I'm after concerns the selectedInfo of the working copy, the lack of comment and clear intentions just let me lost in a desert.
My idea would be to (double) dispatch the search of snapshot to the info, but I have no idea what could be a good candidate implementation.

End of rant!

Le mer. 30 déc. 2020 à 17:52, David T. Lewis <[hidden email]> a écrit :
On Wed, Dec 30, 2020 at 02:57:23PM +0000, [hidden email] wrote:
> A new version of Monticello was added to project The Inbox:
> http://source.squeak.org/inbox/Monticello-nice.734.mcz
>
> ==================== Summary ====================
>
> Name: Monticello-nice.734
> Author: nice
> Time: 30 December 2020, 3:57:21.60758 pm
> UUID: 863f642a-7795-4eb6-aa6b-76a5af3a8504
> Ancestors: Monticello-mt.733
>
> Workaround version history browser bug when 'working copy' is selected.
>
> The snapshot is searched in repositoryGroup, and logically, none is found.
> Note that the self selectedInfo isKindOf: MCWorkingCopyAncestry, which does not behave as a regular MCVersionInfo, it does not even answer to versionName...
> This somehow accelerate the failure thru an ifError: [] handling, but at the end, the selectedSnapshot is still nil which makes the 'view changes from...' menu fail.
>
> The workaround is not very nice, it use implicit knowledge that 'working copy' will be at top of list (index = 1).
>
> If you can think of nicer fix, please raise voice.
>

Attached is another way to do it without relying on the list index. I have
not tested very much but it seems to be all right.

  MCVersionHistoryBrowser>>snapshotForInfo: aVersionInfo
     "Answer snapshot for aVersionInfo or for the working copy if not found"
     ^ (self repositoryGroup
           versionWithInfo: aVersionInfo
              ifNone: [ package ]) snapshot


Apologies for violating MC package standards by including a method comment ;-)

Dave




Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Monticello-nice.734.mcz

timrowledge


> On 2020-12-30, at 10:11 AM, Nicolas Cellier <[hidden email]> wrote:
[eloquent rant elided]

I refer us, as so often, to Adele's Dictum: "If it isn't documented, it doesn't exist. If it doesn't exist, what exactly did we pay you for?"

Of course, part of the problem is that for a lot of things surprisingly important to us *we didn't pay anyone*.

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Thesaurus: Ancient reptile with a truly extensive vocabulary



Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Monticello-nice.734.mcz

marcel.taeumel
In reply to this post by commits-2
Hi Nicolas.

If you can think of nicer fix, please raise voice.

I just put Monticello-mt.734 into the inbox. Please take a look and decide for yourself. :-)

Best,
Marcel

Am 30.12.2020 15:57:30 schrieb [hidden email] <[hidden email]>:

A new version of Monticello was added to project The Inbox:
http://source.squeak.org/inbox/Monticello-nice.734.mcz

==================== Summary ====================

Name: Monticello-nice.734
Author: nice
Time: 30 December 2020, 3:57:21.60758 pm
UUID: 863f642a-7795-4eb6-aa6b-76a5af3a8504
Ancestors: Monticello-mt.733

Workaround version history browser bug when 'working copy' is selected.

The snapshot is searched in repositoryGroup, and logically, none is found.
Note that the self selectedInfo isKindOf: MCWorkingCopyAncestry, which does not behave as a regular MCVersionInfo, it does not even answer to versionName...
This somehow accelerate the failure thru an ifError: [] handling, but at the end, the selectedSnapshot is still nil which makes the 'view changes from...' menu fail.

The workaround is not very nice, it use implicit knowledge that 'working copy' will be at top of list (index = 1).

If you can think of nicer fix, please raise voice.

=============== Diff against Monticello-mt.733 ===============

Item was changed:
----- Method: MCVersionHistoryBrowser>>selectedSnapshot (in category 'accessing') -----
selectedSnapshot
+ index = 1 ifTrue: [^package snapshot].
^ self snapshotForInfo: self selectedInfo!