Camelia, the Perl 6 bug

IRC log for #cdk, 2010-06-10

| Channels | #cdk index | Today | | Search | Google Search | Plain-Text | summary

All times shown according to UTC.

Time Nick Message
05:06 sneumann_ joined #cdk
05:09 egonw joined #cdk
06:29 egonw joined #cdk
06:57 Gpox joined #cdk
07:13 sneumann_ joined #cdk
08:00 egonw CDK-JChemPaint 13 is tagged and uploaded
08:05 CIA-47 org.openscience.cdk: Egon Willighagen bioclipse2.4 * re30c939 / (2 files): Return two missing parameters - http://bit.ly/9V52IJ
08:05 CIA-47 org.openscience.cdk: Egon Willighagen bioclipse2.4 * r80060ee / (39 files in 22 dirs): Uploaded CDK-JChemPaint 13 - http://bit.ly/b71nNa
08:20 jpansanel moin
08:20 zarah oh hai jpansanel
08:20 jpansanel egonw : Mychem is now working fine with MySQL 5.1
08:20 egonw ah, well done!
08:20 egonw jpansanel++
08:20 jpansanel :)
08:21 jpansanel I've too release soon
08:25 sneumann joined #cdk
08:36 CIA-47 org.openscience.cdk: Egon Willighagen bioclipse2.4 * r9d341dc / (12 files in 12 dirs): Uploaded CDK-JChemPaint 14 - http://bit.ly/cBywGG
08:41 jpansanel egonw : the files are on the Mychem trunk
08:48 egonw argh.... no !
08:48 * egonw is a bloody idiot for not taking the time to just start writing unit tests for this...
08:48 * egonw found another regression in 14 ...
08:57 Gpox nothing crusial but when using an interface in a parameter use <? extends ...> that way you can use model.set(...) with a subclass as the value
09:08 egonw_ joined #cdk
09:10 CIA-47 org.openscience.cdk: Egon Willighagen bioclipse2.4 * rb228bea / plugins/org.openscience.cdk.render/src/org/​openscience/cdk/renderer/RendererModel.java : Fixed the getDefault() method; this patch is filed upstream -
09:23 jbrefort joined #cdk
10:01 maclean joined #cdk
10:04 egonw why oh why do we have a HighlightAtomGenerator extends BasicAtomGenerator??? !?!?!?! Arghh ?!?!?
10:04 egonw what if I want to highlight an extended atom then ?!?!?!?
10:04 egonw the current jchempaint implementation makes me cry
10:05 maclean Um.
10:05 * egonw is looking for the HighlightExtendedAtomGenerator ...
10:07 * egonw is implementing getParameters() to also include parameters from super generators
10:07 maclean Right.
10:07 * maclean is still struggling to see the problem though. Maybe I need some tea.
10:09 maclean HighlightAtomGenerator only extends BasicAtomGenerator to use its isHydrogen method
10:09 maclean which is a pretty dumb reason for extending, actually, but it was probably me that did it, sorry.
10:09 egonw indeed
10:09 egonw no worries
10:10 egonw this is no ones fault
10:10 egonw we were on much pressure to deliver
10:10 egonw we made mistakes
10:10 egonw that's normal
10:10 maclean isHydrogen could be a static method of the BasicAtomGenerator?
10:10 maclean That's not a great solution though.
10:11 egonw no, just copy it
10:11 egonw also, those methods must not be public anyway
10:11 egonw they are not supposed to be used outside that class anyway
10:12 maclean yeah, true.
10:12 egonw this is actually very much like the requirement that *every* public method must have a unit test...
10:12 egonw that scares people off making a method public :)
10:14 maclean or writing code with the optimum method density...
10:15 maclean the junit website FAQ has bad things to say about unit testing get/set methods.
10:16 egonw such as?
10:17 maclean that at some point you are just testing the implementation of java
10:17 maclean getName/setName, where name is just a private string field, for example.
10:18 egonw for a pure Java Bean, yes...
10:18 maclean I realise that it protects against changes to the underlying implementation
10:18 egonw but many, many code is not just using the Bean approach...
10:18 egonw where a get/set method actually has side effects, etc...
10:20 maclean hmmm.
10:20 egonw argghhhh....
10:20 egonw the main reason for doing something right from the start, is that you do not have to say arghh all the time... :(
10:21 maclean argh--
10:22 * egonw curses at JChemPaint
10:22 egonw maclean: found more trouble caused by the extending...
10:22 maclean oh?
10:23 egonw parameters.addAll(super.getParamater()) does not work:
10:23 egonw java.lang.UnsupportedOperationException
10:23 egonw at java.util.AbstractList.add(AbstractList.java:148)
10:23 egonw at java.util.AbstractList.add(AbstractList.java:108)
10:23 egonw at java.util.AbstractCollection.add​All(AbstractCollection.java:322)
10:23 egonw at org.openscience.cdk.renderer.generato​rs.HighlightAtomGenerator.getParamete​rs(HighlightAtomGenerator.java:113)
10:24 egonw ok, trying a possible solution...
10:24 maclean ahhh. yes. Collections.addAll(parameters, super.getParameters())
10:24 maclean ?
10:24 egonw perhaps this is caused by Array.asList...
10:24 egonw I'll try that if the current idea does not work
10:25 egonw OK... new ArrayList instead of Arrays.asList solves the problem...
10:25 egonw phew...
10:28 egonw ok, that covers it...
10:28 egonw making patches...
10:28 egonw but first lunch
10:28 maclean hooray for lunch
10:29 egonw :)
10:29 egonw and coffee
10:29 egonw hooray for coffee
10:29 maclean indeed
10:47 CIA-47 org.openscience.cdk: Egon Willighagen bioclipse2.4 * rc27eb5a / (4 files in 3 dirs): Two backported patches from CDK-JChemPaint 15 (or so): return parameters from super generators; more informative IllegalAccessError message - http://bit.ly/cDznsv
10:55 maclean Weird that Arrays.asList returns a list that doesn't support addAll. May be a java bug.
10:56 egonw I guess it returns a final list
10:56 egonw immutable one...
10:56 egonw oh, found more JCP stupidity^Winteresting features...
10:57 egonw why not add another mechanism for setting parameters, like via a constructor ?
10:57 egonw public AtomNumberGenerator(Vector2d offset)
10:57 maclean Heh. Wasn't me. :)
10:58 egonw Gpox: this time it was you ;)
10:58 maclean And these choices were made before the new IGeneratorParameter architecture, so they were sensible at the time.
10:58 maclean I guess.
10:58 egonw well... we already had the RendererModel
10:59 egonw anyway...
10:59 egonw gonna make a patch now...
10:59 egonw since I have to release patch 15 later today anyway...
10:59 egonw I was actually hoping to work on book chapters :(
11:01 egonw btw... it's screen space too...
11:01 egonw so, more scale trouble
11:02 maclean I really think that a chemical drawing package that doesn't allow users to specify sizes in screen space will not be useful.
11:02 maclean I agree that using percentages is more scalable, but still.
11:05 maclean Ahh. now I understand. Arrays.asList returns an instance of Arrays.ArrayList which extends AbstractList but doesn't override the add or addAll methods.
11:06 maclean Only get/set. Documentation bug, really.
11:18 egonw maclean: I agree with the idea that defining some rendering parameters are easier to define in screen coordinates...
11:18 egonw I also think that this can be perfectly done at the application level
11:19 egonw and in that way separate some problems...
11:19 maclean application level =?
11:20 egonw outside render.* and control.*
11:20 egonw an applet, widget, etc can easily define such parameters, and convert those into JCP parameters when needed
11:20 egonw that's what happens now...
11:21 egonw but each time we are drawing
11:21 egonw this makes the whole drawing code a pain to read
11:21 egonw I mean:
11:21 egonw Vector2d offset = new Vector2d(this.offset.x,-this.offset.y);
11:21 egonw WTF???
11:21 egonw what's that - about?
11:21 maclean hmmm.
11:21 egonw there is no JavaDoc, and *no* way to link this to the underlying problem
11:21 egonw this is what causes all those nasty interaction bugs
11:22 egonw when doing 'minor' bug fixes elsewhere
11:22 maclean the offset thing looks a little like the java-coords to molfile-coords thing (*-1)
11:22 egonw "oh, and don't forget to change that - also at location X, Y, Z, and in files A, C, F, and X2 too"
11:23 maclean well, it's a lot better than it used to be :) I remember the old code.
11:24 egonw that's just a matter of time
11:24 egonw the old code used to be readable too
11:24 egonw until extra functionality got added
11:25 maclean but I really like that new functionality is added in a new generator class.
11:26 egonw_ joined #cdk
11:26 maclean and I quite like that just adding a new parameter to that generator will now 'magically' appear in the model.
11:27 egonw indeed
11:28 egonw btw, I am not sure about your alternative solution yet...
11:28 egonw the idea is very good
11:28 egonw but I also like the not having to cast...
11:28 egonw I need more arguments in either direction :)
11:30 maclean Yeah, it's not much of an improvement, really.
11:30 maclean And it's all to get rid of the need to use model.get(Zoom.class) - that is, the ".class" bit.
11:31 maclean Which is quite unusual, and looks strange.
11:31 maclean anyway, egonw : shouldn't this work : "git pull pele tag cdk-jchempaint-12"
11:32 maclean where "pele" is my name for http://pele.farmbio.uu.se/git/cdk-jchempaint.git
11:32 zarah maclean's link is also http://tinyurl.com/2ut9nha
11:34 egonw I think just:
11:34 egonw git pull pele cdk-jchempaint-14
11:34 egonw without the 'tag'
11:35 maclean ah
11:35 egonw the 'classes' are not so large... <1kB
11:35 egonw but still with so many parameters, it's significant...
11:35 egonw not sure how far obfuscating helps here...
11:36 egonw as this 900-ish bytes is mostly class and method names
11:36 maclean well features == size
11:36 egonw true
11:36 maclean but there is overhead with method and class names, yes
11:36 egonw which is why we modularize :)
11:50 maclean egonw : no, sorry, getting "fatal: Couldn't find remote ref refs/tags/cdk-jchempaint-14"
11:50 maclean either with "git pull pele refs/tags/cdk-jchempaint-14"
11:51 maclean or "fatal: Couldn't find remote ref cdk-jchempaint-14"
11:51 maclean with "git pull pele cdk-jchempaint-14"
11:51 egonw mom
11:54 egonw ok, try now
11:54 * egonw ran git update-server-info
11:54 maclean ahhh!
11:55 maclean takk
12:00 egonw maclean: you can pull -15 too now
12:00 maclean ah, cool.
12:01 maclean I assume that it is best to do so in a new branch off master?
12:03 * maclean is git grinding today - level up soon.
12:11 egonw to do what?
12:13 maclean er, sorry. already done now. I am finally doing what I said I would do on Tues (?) - making renderer unit tests
12:15 egonw cool
12:16 maclean so I made a new branch, pulled the cdk-jcp-15 tag, fixed the build.props, fixed the docheck.jar, and I was done! :)
12:17 egonw :)
12:17 egonw yeah, that doccheck removal patch is still not reviewed...
12:17 egonw hint, hint .. :)
12:22 maclean ok, will take a look. it's a trivial patch, isn't it? like removing one line.
12:23 maclean patch tracking with sourceforge is horrible. I click on some random patchfile, it downloads, then I quicklook it in the finder.
12:24 egonw yeah, very trivial patch
12:24 maclean a much nicer solution would be more github-like, with syntax-highlighted files, and so on.
12:24 egonw preferably, you apply it locally and sign it off
12:24 egonw yeah, we want to set up a git repository cdk-staging
12:24 egonw where proposed patches will be uploaded
12:25 maclean that would be cool. how would you separate them out?
12:25 egonw but that takes either much manual work, or development of some better framework
12:25 egonw yes, that's one of the questions then
12:25 maclean yeah, the framework is the problem.
12:26 maclean I have a fantastic vision of a 'feed' of patches coming in from different repositories, and checkboxes, and threaded discussions... oh my.
12:28 egonw :)
12:50 egonw mmm... cannot get compact mode to work :(
12:50 maclean !!
12:50 maclean Compact mode is my favourite!
12:50 maclean (seriously :-|_
12:50 egonw yeah, I like it too
12:51 maclean Uhhhmmm. I think I resolved the problem once by using ExtendedAtomGenerator, rather than Basic.
12:51 maclean Which is bad, of course.
12:54 egonw yes, pink atom numbers!
12:55 maclean Er. Oh good.
12:55 egonw at least something works :)
12:55 maclean I think I may see the problem with isCompact.
12:55 maclean No, wait. I don't/
12:56 maclean but it is one of the ones that is called 'locally' in the generator.
12:56 maclean line 184 : "} else if (isCompact.getValue()) {"
13:01 maclean oh, and egonw : I can't get the 1st doccheck removal patch to apply. I can't see why, either :(
13:01 egonw oh bugger...
13:01 egonw git am -3
13:01 egonw try adding the -3
13:01 maclean well, I was using git apply, but ok
13:03 maclean also, the patch seems to remove the .gitignore file
13:04 maclean ah, right git am lets you sign off on a patch?
13:04 egonw ok... enough review for now...
13:04 egonw seems I need to do some patch cleaning up :)
13:04 egonw yes, think so...
13:04 maclean ok.
13:04 egonw but you can do that afterwards anyway with:
13:04 egonw git commit --append --signoff
13:06 maclean ah, right.
13:20 egonw ok, new CDK-JChemPaint blog post: http://chem-bla-ics.blogspot.com/2010/06/cd​k-jchempaint-6-rendering-atom-numbers.html
13:20 zarah egonw's link is also http://tinyurl.com/2bnf9go
13:21 egonw btw, I just love Git:
13:21 egonw #       renamed:    cdk-1.3.4.jar -> cdk-1.3.5.jar
13:21 egonw #       renamed:    cdk-jchempaint-9.jar -> cdk-jchempaint-15.jar
13:21 egonw #       new file:   renderAtomNumbers.groovy
13:47 maclean Hmmm. So a BasicAtomGenerator.ColorByType and a AtomNumberGenerator.ColorByType.
13:55 egonw yes, because you like to have differences for both?
13:56 egonw or only want to register one of the two generators?
13:58 egonw ah, you're reply came through now...
14:00 maclean no, just thinking aloud.
14:01 maclean the main 'problem'  is that you can't import these classes as static, to use them like model.get(ColorByType), but have to use the fully qualified version.
14:01 maclean but that's not much of a problem.
14:02 egonw yeah, if you use them both within the same .java file, yes
14:02 maclean goddamit eclipse! why do you not recursively refresh!?
14:02 egonw "640kB should be enough for everyone"?
14:02 maclean lolwut?
14:11 maclean egonw : ok, so I made a test for setting the compact shape, and it fails. do you want me to give you failing tests?
14:11 maclean or would you prefer that I fix the failing code :)
14:14 egonw no, please do give me a patch for a failing test too...
14:14 egonw the fix can come later...
14:14 maclean ok
14:14 egonw but the unit test may very well server just as template code for other unit test hackers
14:16 maclean ahhh. false alarm, anyway. compact atom does work.
14:16 maclean you just have to have both "model.set(CompactShape.class, Shape.OVAL)" and "model.set(CompactAtom.class, true)"
14:17 egonw why does it not pick up the default shape?
14:17 egonw is the Shape.SQUARE broken then?
14:18 maclean Oh, I didn't test that one yet.
14:18 maclean mom
14:19 maclean yep, that works
14:36 egonw maclean: pushing 15+ColorByType to pele...
14:36 maclean oh.
14:36 egonw use the 13-unsorted-patches to pull
14:36 maclean okay.
14:41 maclean huh. that actually worked.
14:41 egonw :)
14:56 egonw_ joined #cdk
15:06 maclean erk! you can't use the BasicAtomGenerator by itself without the AtomNumberGenerator.
15:06 maclean (Unless you only generate compact atoms)
15:08 maclean Argh. No, it was an import error. The exact same one I just talked about up there /\ :(
15:20 egonw huh?
15:22 maclean I imported both AtomColorer classes, and got a runtime exception.
15:22 egonw ah, ic
16:36 sneumann joined #cdk
16:59 egonw joined #cdk
17:42 egonw joined #cdk
17:54 carsten joined #cdk
18:09 jbrefort joined #cdk
18:11 sneumann joined #cdk
18:14 sneumann joined #cdk
20:32 jbrefort joined #cdk
22:28 egonw_ joined #cdk
23:53 CIA-47 joined #cdk

| Channels | #cdk index | Today | | Search | Google Search | Plain-Text | summary