[squeak-dev] The Trunk: Tools-eem.1104.mczThiede, Christoph Christoph.Thiede at student.hpi.uni-potsdam.deFri Jan 14 21:53:03 UTC 2022
Hi Marcel, 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 ... > 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. 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. :-) > I would argue that MenuBuilderFailed is easier to maintain. :-) Well, I wrote these lines, but to me, it reads rather straightforward. We already use this pattern in the inspector (see senders of #streamErrorDoing:on:). 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. Best, Christoph ________________________________ Von: Squeak-dev <squeak-dev-bounces at lists.squeakfoundation.org> im Auftrag von Taeumel, Marcel Gesendet: Freitag, 14. Januar 2022 17:13:11 An: squeak-dev Betreff: Re: [squeak-dev] The Trunk: Tools-eem.1104.mcz Hi Christoph -- -1 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. 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. :-) 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. Best, Marcel Am 14.01.2022 17:02:39 schrieb christoph.thiede at student.hpi.uni-potsdam.de <christoph.thiede at student.hpi.uni-potsdam.de>: Hi Marcel, 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. :-) =============== Summary =============== Change Set: menuBuilderFailed Date: 14 January 2022 Author: Christoph Thiede 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. =============== Diff =============== Model>>buildMenu:withBuilders:shifted: {*Tools-pluggable menus} ยท ct 1/14/2022 16:54 (changed) buildMenu: aMenu withBuilders: builders shifted: aBoolean " We let every builder modify the menu. Skip erroneous builders silently. The builder should indicate whether to abort by returning nil." | menu | menu := aMenu. - builders do: [:builder | - menu := [self perform: builder method selector withEnoughArguments: { menu . aBoolean }] - ifError: [:msg | Transcript showln: 'Menu builder failed: ', msg. menu]. - menu ifNil: [^ aMenu]]. - ^ menu + builders do: [:builder | | buildBlock | + buildBlock := [menu := self perform: builder method selector withEnoughArguments: {menu . aBoolean}]. + buildBlock + on: Error, Warning, Halt do: [:ex | + menu add: ('<menu builder failed: {1}>' translated format: {ex}) action: buildBlock]]. + ^ menu Thank you for the discussion, and sorry for not better coordinating this issue. :-) Best, Christoph --- Sent from Squeak Inbox Talk<https://github.com/hpi-swa-lab/squeak-inbox-talk> On 2022-01-14T16:50:51+01:00, marcel.taeumel at hpi.de wrote: > Hi Christoph -- > > Done. See Tools-mt.1105. > > Best, > Marcel > Am 14.01.2022 16:20:46 schrieb Marcel Taeumel <marcel.taeumel at hpi.de>: > Hi Christoph -- > > > This is an anti-pattern in my opinion and should be revised. > > Can we please remove that #ifError: from Model>>#buildMenu:withBuilders:shifted: again? :-) > > 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 =) > > > Or maybe a separate "release mode" preference. > > Seriously ... we do need to talk about "modes" at some point ... Stop it. xD > > Best, > Marcel > Am 14.01.2022 16:16:54 schrieb christoph.thiede at student.hpi.uni-potsdam.de <christoph.thiede at student.hpi.uni-potsdam.de>: > Hi Marcel, > > thank you for the clarification (and thank you Eliot for fixing this bug)! > > 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. :-) > > Can we please remove that #ifError: from Model>>#buildMenu:withBuilders:shifted: again? :-) > > Best, > Christoph > > --- > Sent from Squeak Inbox Talk [https://github.com/hpi-swa-lab/squeak-inbox-talk] > > On 2022-01-14T10:08:23+01:00, marcel.taeumel at hpi.de wrote: > > > Hi Christoph -- > > > > You are showing the "more..." menu. The initial menu is truncated because of that bug in #isBreakOnEntry. > > > > > > > > After Eliot's patch, it looks as expected: > > > > > > > > Best, > > Marcel > > Am 13.01.2022 19:26:32 schrieb Thiede, Christoph <christoph.thiede at student.hpi.uni-potsdam.de>: > > I have a message trace here with no message selected, but the menu can be opened anyway without an error: > > > > > > > > > > > > Under which circumstances did this error occur? :-) > > > > Best, > > Christoph > > Von: Squeak-dev <squeak-dev-bounces at lists.squeakfoundation.org> im Auftrag von commits at source.squeak.org <commits at source.squeak.org> > > Gesendet: Donnerstag, 13. Januar 2022 19:21:13 > > An: squeak-dev at lists.squeakfoundation.org; packages at lists.squeakfoundation.org > > Betreff: [squeak-dev] The Trunk: Tools-eem.1104.mcz > > > > Eliot Miranda uploaded a new version of Tools to project The Trunk: > > http://source.squeak.org/trunk/Tools-eem.1104.mcz [http://source.squeak.org/trunk/Tools-eem.1104.mcz] > > > > ==================== Summary ==================== > > > > Name: Tools-eem.1104 > > Author: eem > > Time: 13 January 2022, 10:21:10.513361 am > > UUID: 1d051d8d-473d-43ee-9628-d33992a5849f > > Ancestors: Tools-mt.1103 > > > > Fix MNU in menu building for MessageTrace with no method selected > > > > =============== Diff against Tools-mt.1103 =============== > > > > Item was changed: > > ----- Method: CodeHolder>>isBreakOnEntry (in category 'breakpoints') ----- > > isBreakOnEntry > > > > + ^self selectedClassOrMetaClass > > + ifNil: [false] > > + ifNotNil: > > + [:class| > > + (class > > + compiledMethodAt: self selectedMessageName > > + ifAbsent: nil) > > + ifNil: [false] > > + ifNotNil: [:method| method hasBreakpoint]]! > > - | selectedMethod | > > - selectedMethod := self selectedClassOrMetaClass > > - compiledMethodAt: self selectedMessageName > > - ifAbsent: [^ false]. > > - ^ selectedMethod hasBreakpoint! > > > > > > -------------- next part -------------- > > An HTML attachment was scrubbed... > > URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20220114/a8d504cb/attachment-0001.html> > > -------------- next part -------------- > > A non-text attachment was scrubbed... > > Name: image.png > > Type: image/png > > Size: 22635 bytes > > Desc: not available > > URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20220114/a8d504cb/attachment-0005.png> > > -------------- next part -------------- > > A non-text attachment was scrubbed... > > Name: pastedImage.png > > Type: image/png > > Size: 4074 bytes > > Desc: not available > > URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20220114/a8d504cb/attachment-0006.png> > > -------------- next part -------------- > > A non-text attachment was scrubbed... > > Name: pastedImage.png > > Type: image/png > > Size: 18275 bytes > > Desc: not available > > URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20220114/a8d504cb/attachment-0007.png> > > -------------- next part -------------- > > A non-text attachment was scrubbed... > > Name: pastedImage.png > > Type: image/png > > Size: 15326 bytes > > Desc: not available > > URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20220114/a8d504cb/attachment-0008.png> > > -------------- next part -------------- > > A non-text attachment was scrubbed... > > Name: image.png > > Type: image/png > > Size: 78379 bytes > > Desc: not available > > URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20220114/a8d504cb/attachment-0009.png> > > > > > -------------- next part -------------- > An HTML attachment was scrubbed... > URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20220114/c96d0a4c/attachment.html> > > ["menuBuilderFailed.1.cs"] -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20220114/a8e4f243/attachment-0001.html>
More information about the Squeak-dev mailing list |
