<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<style type="text/css" style="display:none;"><!-- P {margin-top:0;margin-bottom:0;} --></style>
</head>
<body dir="ltr">
<div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Helvetica,sans-serif;" dir="ltr">
<div id="divtagdefaultwrapper" dir="ltr" style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;">
<p>Hi Marcel,</p>
<p><br>
</p>
<p>sorry for the race condition. If I had come up with this idea earlier, I would of course not have withheld that, of course. Also, I was not expecting you to prepare a solution at the same, I assumed we were still being in the brainstorming phase. :-) Your
first solution was definitely already an improvement, but ideally, it is never too late to discuss a counterproposal ...</p>
<p><br>
</p>
<p>> <span style="font-size:12pt">you changed "ifError" to deal with "Error, Warning, Halt", which is clearly a change of the existing semantics. Baby steps. I would not want to mask a Halt here.</span></p>
<div><br>
</div>
<div>I tried to reuse the pattern known from "draw errors" and "layout errors". After all, I thought the goal was not to disturb uninterested users with "debuggers in production"? What kind of strictness do we prefer? Let me know and I am happy to adjust the
solution again. :-)</div>
<div><br>
</div>
<div>> <span>I would argue that MenuBuilderFailed is easier to maintain. :-)</span></div>
<div><span><br>
</span></div>
<div><span>Well, I wrote these lines, but to me, it reads rather straightforward. We already use this pattern in the inspector (see senders of #<span>streamErrorDoing:on:).</span> I'm not sure whether nested exception signaling has a lower code complexity.
And finally, the motivation for my proposal was not to disturb users with unnecessary debuggers, just like we do in the inspector and in the explorer.</span></div>
<div><span><br>
</span></div>
<div><span>Best,</span></div>
<div><span>Christoph</span></div>
<div><span><br>
</span></div>
<p></p>
</div>
<hr tabindex="-1" style="display:inline-block; width:98%">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" color="#000000" style="font-size:11pt"><b>Von:</b> Squeak-dev <squeak-dev-bounces@lists.squeakfoundation.org> im Auftrag von Taeumel, Marcel<br>
<b>Gesendet:</b> Freitag, 14. Januar 2022 17:13:11<br>
<b>An:</b> squeak-dev<br>
<b>Betreff:</b> Re: [squeak-dev] The Trunk: Tools-eem.1104.mcz</font>
<div> </div>
</div>
<div>
<div id="__MailbirdStyleContent" dir="ltr" style="font-size:10pt; font-family:Arial; color:#000000; text-align:left">
Hi Christoph --
<div><br>
</div>
<div>-1</div>
<div><br>
</div>
<div>First, you changed "ifError" to deal with "Error, Warning, Halt", which is clearly a change of the existing semantics. Baby steps. I would not want to mask a Halt here.</div>
<div><br>
</div>
<div>Second, your solution has a more complex control flow due to the indirection of buildBlock and the repeated references in the (erroneous) menu items. I would argue that MenuBuilderFailed is easier to maintain. :-)</div>
<div><br>
</div>
<div>Third, I don't care. This ridiculous. You could just have proposed a solution instead of complaining in some other thread, then letting me work on it, creating a race condition.</div>
<div><br>
</div>
<div>Best,</div>
<div>Marcel</div>
<div class="mb_sig"></div>
<blockquote class="history_container" type="cite" style="border-left-style:solid; border-width:1px; margin-top:20px; margin-left:0px; padding-left:10px; min-width:500px">
<p style="color:#AAAAAA; margin-top:10px">Am 14.01.2022 17:02:39 schrieb christoph.thiede@student.hpi.uni-potsdam.de <christoph.thiede@student.hpi.uni-potsdam.de>:</p>
<div style="font-family:Arial,Helvetica,sans-serif">Hi Marcel,<br>
<br>
argh, a few minutes too late, but here is an alternative proposal that comes without the overhead of a new exception class and extra state that needs to be maintained. I would also call this approach more user-friendly because you do not see a single debugger
without requesting it. :-)<br>
<br>
<b><span style="font-family:Bitmap DejaVu Sans">=============== Summary ===============</span></b><span style="font-family:Bitmap DejaVu Sans"><br>
<br>
Change Set: menuBuilderFailed<br>
Date: 14 January 2022<br>
Author: Christoph Thiede<br>
<br>
This change set increases the discoverability of errors that have occured inside a menu builder. Instead of aborting the menu construction and logging the incident to the Transcript only, a failure item is added to the menu. The user can click on this item
to debug the underlying error.<br>
<br>
</span><b><span style="font-family:Bitmap DejaVu Sans">=============== Diff ===============</span></b><span style="font-family:Bitmap DejaVu Sans"><br>
<br>
</span><b><span style="font-family:Bitmap DejaVu Sans">Model>>buildMenu:withBuilders:shifted: {*Tools-pluggable menus} · ct 1/14/2022 16:54 (changed)</span></b><span style="font-family:Bitmap DejaVu Sans"><br>
buildMenu: aMenu withBuilders: builders shifted: aBoolean<br>
" We let every builder modify the menu. Skip erroneous builders silently.<br>
The builder should indicate whether to abort by returning nil."<br>
| menu |<br>
menu := aMenu.<br>
</span><s><span style="color:#0000FF"><span style="font-family:Bitmap DejaVu Sans">- builders do: [:builder |<br>
- menu := [self perform: builder method selector withEnoughArguments: { menu . aBoolean }]<br>
- ifError: [:msg | Transcript showln: 'Menu builder failed: ', msg. menu].<br>
- menu ifNil: [^ aMenu]].<br>
- ^ menu<br>
</span></span></s><span style="color:#FF0000"><span style="font-family:Bitmap DejaVu Sans">+ builders do: [:builder | | buildBlock |<br>
+ buildBlock := [menu := self perform: builder method selector withEnoughArguments: {menu . aBoolean}].<br>
+ buildBlock<br>
+ on: Error, Warning, Halt do: [:ex |<br>
+ menu add: ('<menu builder failed: {1}>' translated format: {ex}) action: buildBlock]].<br>
+ ^ menu<br>
<br>
</span></span><span style="font-family:Bitmap DejaVu Sans">Thank you for the discussion, and sorry for not better coordinating this issue. :-)</span><span style="color:#FF0000"><span style="font-family:Bitmap DejaVu Sans"><br>
</span></span><span style="font-family:Bitmap DejaVu Sans"><br>
Best,<br>
Christoph</span><br>
<br>
<span style="color:#808080">---<br>
</span><span style="color:#808080"><i>Sent from </i></span><span style="color:#808080"><i><a href="https://github.com/hpi-swa-lab/squeak-inbox-talk"><u><span style="color:#808080">Squeak Inbox Talk</span></u></a></i></span><br>
<br>
On 2022-01-14T16:50:51+01:00, marcel.taeumel@hpi.de wrote:<br>
<br>
> Hi Christoph --<br>
> <br>
> Done. See Tools-mt.1105.<br>
> <br>
> Best,<br>
> Marcel<br>
> Am 14.01.2022 16:20:46 schrieb Marcel Taeumel <marcel.taeumel at hpi.de>:<br>
> Hi Christoph --<br>
> <br>
> > This is an anti-pattern in my opinion and should be revised.<br>
> > Can we please remove that #ifError: from Model>>#buildMenu:withBuilders:shifted: again? :-)<br>
> <br>
> Not every user is a developer. Having menus that do not throw errors is a good thing. Hmm... a specialized Warning about the broken menu seems nice =)<br>
> <br>
> > Or maybe a separate "release mode" preference.<br>
> <br>
> Seriously ... we do need to talk about "modes" at some point ... Stop it. xD<br>
> <br>
> Best,<br>
> Marcel<br>
> Am 14.01.2022 16:16:54 schrieb christoph.thiede at student.hpi.uni-potsdam.de <christoph.thiede at student.hpi.uni-potsdam.de>:<br>
> Hi Marcel,<br>
> <br>
> thank you for the clarification (and thank you Eliot for fixing this bug)!<br>
> <br>
> So the menu building mechanism is actually ignoring errors from single menu builders? This is an anti-pattern in my opinion and should be revised. Silently and unconditionally dropping errors instead of reporting them to the user/developer (and writing them
to the Transcript does *not* count as reporting IMHO because I only open the Transcript when I *expect* to see particular messages in it) looks like a bad practice to me that makes debugging and testing (both manually and automated) unnecessarily hard and
non-interactive. It's not the Smalltalk way of facilitating short feedback cycles and direct manipulation. If an error occurs, I want to know about it and fix it immediately (which usually only takes a few seconds or minutes). If you really see some situations
where fixing the error is so expensive (I would be curious to hear them), I would prefer talking about an opt-in mechanism similar to Warning class >> #suppressed:. Or maybe a separate "release mode" preference. But please let's keep our primary focus in the
main image on developers that are curious to notice and report/resolve every bug. :-)<br>
> <br>
> Can we please remove that #ifError: from Model>>#buildMenu:withBuilders:shifted: again? :-)<br>
> <br>
> Best,<br>
> Christoph<br>
> <br>
> ---<br>
> Sent from Squeak Inbox Talk [https://github.com/hpi-swa-lab/squeak-inbox-talk]<br>
> <br>
> On 2022-01-14T10:08:23+01:00, marcel.taeumel at hpi.de wrote:<br>
> <br>
> > Hi Christoph --<br>
> ><br>
> > You are showing the "more..." menu. The initial menu is truncated because of that bug in #isBreakOnEntry.<br>
> ><br>
> ><br>
> ><br>
> > After Eliot's patch, it looks as expected:<br>
> ><br>
> ><br>
> ><br>
> > Best,<br>
> > Marcel<br>
> > Am 13.01.2022 19:26:32 schrieb Thiede, Christoph <christoph.thiede at student.hpi.uni-potsdam.de>:<br>
> > I have a message trace here with no message selected, but the menu can be opened anyway without an error:<br>
> ><br>
> ><br>
> > <br>
> ><br>
> ><br>
> > Under which circumstances did this error occur? :-)<br>
> ><br>
> > Best,<br>
> > Christoph<br>
> > Von: Squeak-dev <squeak-dev-bounces at lists.squeakfoundation.org> im Auftrag von commits at source.squeak.org <commits at source.squeak.org><br>
> > Gesendet: Donnerstag, 13. Januar 2022 19:21:13<br>
> > An: squeak-dev at lists.squeakfoundation.org; packages at lists.squeakfoundation.org<br>
> > Betreff: [squeak-dev] The Trunk: Tools-eem.1104.mcz<br>
> > <br>
> > Eliot Miranda uploaded a new version of Tools to project The Trunk:<br>
> > http://source.squeak.org/trunk/Tools-eem.1104.mcz [http://source.squeak.org/trunk/Tools-eem.1104.mcz]<br>
> ><br>
> > ==================== Summary ====================<br>
> ><br>
> > Name: Tools-eem.1104<br>
> > Author: eem<br>
> > Time: 13 January 2022, 10:21:10.513361 am<br>
> > UUID: 1d051d8d-473d-43ee-9628-d33992a5849f<br>
> > Ancestors: Tools-mt.1103<br>
> ><br>
> > Fix MNU in menu building for MessageTrace with no method selected<br>
> ><br>
> > =============== Diff against Tools-mt.1103 ===============<br>
> ><br>
> > Item was changed:<br>
> > ----- Method: CodeHolder>>isBreakOnEntry (in category 'breakpoints') -----<br>
> > isBreakOnEntry<br>
> > <br>
> > + ^self selectedClassOrMetaClass<br>
> > + ifNil: [false]<br>
> > + ifNotNil:<br>
> > + [:class|<br>
> > + (class<br>
> > + compiledMethodAt: self selectedMessageName<br>
> > + ifAbsent: nil)<br>
> > + ifNil: [false]<br>
> > + ifNotNil: [:method| method hasBreakpoint]]!<br>
> > - | selectedMethod |<br>
> > - selectedMethod := self selectedClassOrMetaClass<br>
> > - compiledMethodAt: self selectedMessageName<br>
> > - ifAbsent: [^ false].<br>
> > - ^ selectedMethod hasBreakpoint!<br>
> ><br>
> ><br>
> > -------------- next part --------------<br>
> > An HTML attachment was scrubbed...<br>
> > URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20220114/a8d504cb/attachment-0001.html><br>
> > -------------- next part --------------<br>
> > A non-text attachment was scrubbed...<br>
> > Name: image.png<br>
> > Type: image/png<br>
> > Size: 22635 bytes<br>
> > Desc: not available<br>
> > URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20220114/a8d504cb/attachment-0005.png><br>
> > -------------- next part --------------<br>
> > A non-text attachment was scrubbed...<br>
> > Name: pastedImage.png<br>
> > Type: image/png<br>
> > Size: 4074 bytes<br>
> > Desc: not available<br>
> > URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20220114/a8d504cb/attachment-0006.png><br>
> > -------------- next part --------------<br>
> > A non-text attachment was scrubbed...<br>
> > Name: pastedImage.png<br>
> > Type: image/png<br>
> > Size: 18275 bytes<br>
> > Desc: not available<br>
> > URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20220114/a8d504cb/attachment-0007.png><br>
> > -------------- next part --------------<br>
> > A non-text attachment was scrubbed...<br>
> > Name: pastedImage.png<br>
> > Type: image/png<br>
> > Size: 15326 bytes<br>
> > Desc: not available<br>
> > URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20220114/a8d504cb/attachment-0008.png><br>
> > -------------- next part --------------<br>
> > A non-text attachment was scrubbed...<br>
> > Name: image.png<br>
> > Type: image/png<br>
> > Size: 78379 bytes<br>
> > Desc: not available<br>
> > URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20220114/a8d504cb/attachment-0009.png><br>
> ><br>
> ><br>
> -------------- next part --------------<br>
> An HTML attachment was scrubbed...<br>
> URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20220114/c96d0a4c/attachment.html><br>
> <br>
> <br>
["menuBuilderFailed.1.cs"]</div>
</blockquote>
</div>
</div>
</div>
</body>
</html>