On Thu, Jan 14, 2016 at 7:59 PM, David Allouche <da...@allouche.net> wrote: > On 14 Jan 2016, at 04:26, Ben Coman <b...@openinworld.com> wrote: > hi David, > Since you seem keen to jump into some code reviews, just > a prompt you might like to make this your first one... > https://pharo.fogbugz.com/default.asp?17363 > >> ---------- Forwarded message ---------- >> From: Stephan Eggermont <step...@stack.nl> >> Date: Thu, Jan 14, 2016 at 1:09 AM >> Subject: Re: [Pharo-dev] Bad layoutFrame>>#hash >> To: pharo-dev@lists.pharo.org >> >> SLICE-Issue-17363-LayoutFrame-fractions-0--0-corner-1--1--LayoutFrame-identity-should-be-true-and-it-is-not-StephanEggermont.2 >> >> Better hash both in distribution and in working with Float coordinates >> > > Thank you. That should be easy, since it was a change I suggested and which > was already reviewed informally on the ML :-) > > I think I figured out how it's supposed to work. Can you confirm?
First let me say ouch! and secondly I'm impressed you typed that all out in detail. It portrays a much better picture than a general comment. So it seems we have some tuning to do. > From the issue page, copy the slice name: > SLICE-Issue-17363-LayoutFrame-fractions-0--0-corner-1--1--LayoutFrame-identity-should-be-true-and-it-is-not-StephanEggermont.2 > Go to the image. Open Monticello Browser. > In right (repositories) pane, select > http://smalltalkhub.com/mc/Pharo/Pharo50Inbox/main > Try double clicking, see that it does not work This is probably a good idea to have. Give a chance for others to comment, then could you open an issue for it on Fogbugz (Milestone=Pharo5, Category=Enhancement, Project=Monticello) > Guess that one should click the Open button in the top right of the > Monticello Browser window. > Come back to the web browser to copy the slice name again, because, I used > the clipboard to paste the repository name in this email. > Paste the slice name in the search pane on the right. > See no result, because there was a trailing newline in the clipboard. I guess this field should exclude returns and even all trailing whitespace. Maybe this should be the default for all such controls (?) Could you open another issue (or actually search a bit first see if its already been noted) > Click on the search pane. Notice it selected all. Click again to put the > caret at the end. > Backspace, okay, one result, that should hopefully be it, but it's too long > to fit in the window. I guess its not intuitive, but I only ever type the Issue Number of the slice, since that is unique and there is usually only a couple of items > Double click on it, nothing happens. Choosing a default double-click action would need some community discussion. Maybe <Merge> ?? > Remember that thing appears to think double clicks do not matter. > Right click on it, see only sorting options, nothing relevant. I don't think I have ever used the sort options. Does anyone else use them so often they couldn't be a submenu of a context menu? > Look at the buttons on the top of the window. Browse, maybe? > Get confused by a class browser that somehow does not have syntax > highlighting, and also shows a lot of things that have nothing to do with > the code I want to review. That was probably not the right button. Close the > window. I never use <Browse>. I'm no sure of its value but note however that Monticello is a general tool for loading packages, which has been pressed into service to load slices (which is just a package binding a set of dependent packages together.) <Browse> might be good for exploring a completely new package before its loaded the first time. > Click on History. Get a window with only the description of the selected > slice, what use could that possibly have? Not so much for reviewing slices which have a shallow history. But again Monticello is a general package management tool. For this, search on 'strings' , then select package Collections-Strings. Observe how version Lorenzano.385 is underlined. This version is the one currently loaded in the image. Observe also Polito.383 is bolded. This is not an an ancestor of the currently loaded package version. You can see Lorenzano.385 has ancestor Lorenzano.384 which has ancestor Lorenzano.383. Select Lorenzano.385 and click <History> and you will see Polito.383 missing from the list. > Click on Changes. Stuff blinks on screen for a couple seconds, to fast to > read. Lucky you ;) it takes a while from Australia. I'm not sure of best practice, but personally, to review slices I only ever use <Merge> in the repository window since this provides a three-way diff. I'm not sure... but I don't think <Changes> does (?) Back when I started with Pharo using this repository <Changes> button gave different results depending on factors I couldn't track down. I've never had a problem with <Merge> and <Merge> is what the CI monkey uses. Now I regularly use the <Changes> button back in the first Monticello window. It compares to the selected repository, so as Stef mentioned on Fogbugz, when you do come to submit your first slice select to compare Pharo50/main rather than Pharo50Inbox/main. If its the first time this release cycle a package is updated, the ancestor won't be in Inbox and so every method is counted as a change (probably it should be better) btw, the CI monkey copies integrated packages from the latter to the former. > I guess that means to reassure me. I wonder how hard it would be to > have a single progress bar. I don't know. Others might chip in. > A window opens with three panes. Too narrow to read. > Maximize the window. Still to narrow. > Maximize the world window. Did not change the size of the window. > Maximize the window again. Weird that changed the size and position a bit. I > guess that unmaximized that window that was not really maximized. > Maximize the window again. Okay now I can see the diff. Interesting find. You probably worked this out, but just to be clear... When the window was first maximized it recorded this was its state, and that was not reset when the world was maximized. So the next click of the button unmaximized it. And the final click properly maximized. So I see two alternatives: * a maximized window should follow the resizing of the world's native window * the maximized state of windows is reset when the world is resized. Lets see what others think. > Okay now, how I am supposed to post comments? I guess I should copy the > relevant bit of code, paste it in the browser, and add my comment. That's > going to make reviewing larger changes painful, but that's something, I > guess. We have some way to go here. Some innovation is needed. > Mh, actually that turned into a step by step account of all the trouble I > had. > > And that's only this long because I already figured how to get a working dev > image (needing the source files was not really obvious at first), and some > notion about finding changes in the Pharo50Inbox. None of which is obvious > to the aspiring contributor nor simply explained on a "how to contribute" > page. Thanks for sticking with it. Human nature is to adapt, and after a while become oblivious to these issues - like automatically stepping over that wobbly cobblestone in your front path without thinking about it. Fresh eyes are really valuable. In case its useful to adap, I'll summarize the workflow I've optimised for me personally: 1. Refresh PharoLauncher. 2. Right-click on latest numeric build and <Create Image> 3. For image name keep build number and replace left with issue number. e.g. "Case17363-50526". Sometimes I append the issue title from fogbugz. Sometimes I need a few images for a case e.g. "Case17363-50526b" usually created from "Local" templates, and its annoying when I forget. 4. Open Image, Monticello, Pharo5Inbox repository. The issue number shows in the native window title, so type that into search. 5. Select latest slice and <Merge> to review changes. I don't proceed with pushing the final execute <Merge> button in this window. I leave it open so after merging I retain a list of the changes. 6. From first Monticello window open a second Pharo5Inbox repository window. Select same slice and <Merge>, then push final <Merge> button to execute. ((For a long time I've been meaning to put a "Keep open" checkbox next to the final <Merge> execute button so I don't need to do this dance. Maybe soon)) 7. Browse from listing changes in retained window, insert appropriate breakpoints to understand the changes and form the questions to ask to learn more. > > Feel free to forward this message to the mailing list. I answered privately > because I was not sure exactly why you sent your message privately. Your new here. I didn't want you to feel pressured being publicly volunteered for something you weren't ready for. But obviously your up for it, so cool. cheers -ben > > Now, I'll post my comment on the slice on fogzbugz.
