Wiki source code of IRC Archive for channel #xwiki
Last modified by Vincent Massol on 2012/10/18 19:19
Show last authors
author | version | line-number | content |
---|---|---|---|
1 | 01:09 <pturcotte> has joined #xwiki | ||
2 | 01:20 <jvdrean> has quit | ||
3 | 01:43 <rrodriguez> has quit | ||
4 | 04:11 <+mflorea> sdumitriu: ping | ||
5 | 04:12 <@sdumitriu> Pong | ||
6 | 04:14 <@sdumitriu> mflorea: You're up early | ||
7 | 04:14 <+mflorea> :) | ||
8 | 04:14 <+mflorea> I haven't slept | ||
9 | 04:14 <+mflorea> would be awesome if you can find the time to review my last two commits (real commits, not the merge) on feature-sheets branch, see https://github.com/xwiki/xwiki-platform/commits/feature-sheets | ||
10 | 04:14 <+mflorea> this one is the most important https://github.com/xwiki/xwiki-platform/commit/aca51893724cb5203e5c7f5cbecdd62d91ec0a1c | ||
11 | 04:15 <@sdumitriu> OK | ||
12 | 04:15 <@sdumitriu> After that, you're all set for this release | ||
13 | 04:15 <@sdumitriu> ? | ||
14 | 04:15 <+mflorea> from the second one https://github.com/xwiki/xwiki-platform/commit/783b42800eb70d883c2b6cc8383dfea2d1026761 the SheetDocumentDisplayer class is interesting | ||
15 | 04:17 <+mflorea> yes, I'm 90% done. What I still need to do is revert some changes to the velocity templates that I previously did on feature-sheets branch and that I don't need anymore (because the sheet is now automatically applied when getRenderedContent is called). | ||
16 | 04:17 <+mflorea> but I'll do this after I sleep a few hours | ||
17 | 04:21 <@sdumitriu> OK Marius | ||
18 | 04:22 <@sdumitriu> I hope at least you're not still at the office | ||
19 | 04:22 <+mflorea> btw, if you want to test on a 3.2 snapshot, you need _only_ the jars: xwiki-platform-legacy-oldcore (including of course the modified xwiki-platform-oldcore), xwiki-platform-display, xwiki-platform-sheet-api and xwiki-platform-rendering-macro-include. You can test by editing Blog.BlogIntroduction and deleting the content (the include macro) | ||
20 | 04:22 <+mflorea> nop :) I'm at home | ||
21 | 04:23 <+mflorea> ok, going to sleep | ||
22 | 04:23 <@sdumitriu> Bye | ||
23 | 04:24 <mflorea> has quit | ||
24 | 04:30 <Denis> has quit | ||
25 | 04:30 <DrLou> has joined #xwiki | ||
26 | 04:42 <DrLou> has quit | ||
27 | 07:46 <mflorea> has joined #xwiki | ||
28 | 07:46 <@sdumitriu> mflorea: Up already? | ||
29 | 07:46 <+mflorea> :) | ||
30 | 07:46 <+mflorea> yep | ||
31 | 07:47 <finbrein> has joined #xwiki | ||
32 | 07:47 <@sdumitriu> Good, I have a question | ||
33 | 07:47 <+mflorea> shoot | ||
34 | 07:47 <@sdumitriu> How does a blog post know which sheet to use? | ||
35 | 07:47 <@sdumitriu> I don't see anything in the blog post document, in the sheet, or in the class | ||
36 | 07:50 <+mflorea> The sheet manager knows: the blog post has an object of type BlogPostClass, and the manager looks for a sheet for this class. If it can't find any explicit bindings it fall-back on naming convention BlogPostClass -> BlogPostSheet and since BlogPostSheet exists it assumes it is the expected sheet | ||
37 | 07:51 <@sdumitriu> Ah, indeed | ||
38 | 07:51 <@sdumitriu> Forgot about that | ||
39 | 08:06 <@cjdelisle> http://blogs.davenportlibrary.com/kids/wp-content/calvin-hobbes-christmas-tree.gif | ||
40 | 08:20 <@sdumitriu> Mixing git index-filter, git update-index, bash, sed, regular expressions... | ||
41 | 08:20 <@sdumitriu> This is getting interesting | ||
42 | 08:30 <vmassol> has joined #xwiki | ||
43 | 08:38 <tmortagne> has joined #xwiki | ||
44 | 09:20 <@sdumitriu> vmassol: Finally, I have something to extract a module with a clean history | ||
45 | 09:20 <@sdumitriu> A bit faster than tree-filter | ||
46 | 09:21 <@sdumitriu> mflorea: I'm going to catch some sleep, I'll continue to review tomorrow | ||
47 | 09:21 <@sdumitriu> (later today for you) | ||
48 | 09:21 <+mflorea> great. I'm cleaning up my changes in the mean time (especially the velocity template changes) | ||
49 | 09:22 <@sdumitriu> By the way, I use this to see all the changes: | ||
50 | 09:22 <@sdumitriu> https://github.com/xwiki/xwiki-platform/compare/master...feature-sheets | ||
51 | 09:22 <+mflorea> yep, me too | ||
52 | 09:22 <+mflorea> I'm going now through all the changes | ||
53 | 09:24 <@cjdelisle> I think I found the problem with panels on myxwiki, someone's redefining panelheader and it's breaking it for everyone. | ||
54 | 09:25 <@sdumitriu> cjdelisle: Yes, it's known | ||
55 | 09:25 <@sdumitriu> It's old | ||
56 | 09:25 <@sdumitriu> Tried to find who exactly redefines it but couldn't find anything | ||
57 | 09:25 <@sdumitriu> I searched in the database files (on the filesystem) directly without any luck | ||
58 | 09:27 <@cjdelisle> it's breaking the update to 3.2 royally | ||
59 | 09:28 <@sdumitriu> Why? It used to be like this since a long time ago | ||
60 | 09:28 <@sdumitriu> And the only solution is to restart tomcat | ||
61 | 09:28 <@cjdelisle> rewriting the macros in syntax 2.0 makes them fail miserably | ||
62 | 09:29 <@cjdelisle> and they are in syntax2.0 in the version 3.2 | ||
63 | 09:29 <sdumitriu> has quit | ||
64 | 09:30 <+tmortagne> s/macros/panels/ ? | ||
65 | 09:32 <@cjdelisle> it's the #panelheader macro that's overridden somewhere | ||
66 | 09:32 <@cjdelisle> I thought that had been fixed | ||
67 | 09:33 <+tmortagne> yes it should be fixed | ||
68 | 09:33 <+tmortagne> normally each page load macros in its own velocity namespace | ||
69 | 09:33 <+tmortagne> and pretty sure there is unit tests for it | ||
70 | 09:33 <@cjdelisle> maybe it's a syntax1.0 thing? | ||
71 | 09:34 <+tmortagne> supposed to be the same whatever the syntaxe | ||
72 | 09:34 <+tmortagne> it's done at getRenderedContent level | ||
73 | 09:34 <+tmortagne> hmm wodering if that's really done properly for panels actually | ||
74 | 09:34 <@cjdelisle> anyway I thought overriding anything in macros.vm was explicitly forbidden | ||
75 | 09:35 <+tmortagne> cjdelisle: not sure there is really a way to explocitely forbid that | ||
76 | 09:35 <@cjdelisle> {{velocity}}{{{ #panelheader('hai') }}}{{/velocity}} pasted into a page shows the overridden version | ||
77 | 09:36 <+tmortagne> it depends what namespace is use for where this velocity macro is redifined | ||
78 | 09:37 <+tmortagne> if it's root namespace then yes you are going to see in even in pages | ||
79 | 09:37 <+tmortagne> that would indicate it's not in a page | ||
80 | 09:38 <+tmortagne> but I don't see where else | ||
81 | 09:38 <@cjdelisle> I wonder if it's in a skin override attachment? | ||
82 | 09:38 <+tmortagne> even if that would explain why sdumitriu ccould not find it in the db | ||
83 | 09:38 <@cjdelisle> I don't really understand skin attachments, they are zip files? | ||
84 | 09:38 <+tmortagne> maybe, not sure what is used to execute skin overrides | ||
85 | 09:41 <@cjdelisle> if they are zipped then that would at least explain why it was never found. | ||
86 | 09:43 <+tmortagne> in any case if anyone can redefine a farm velocity macro in his subwiki skin zipped or not it's a bug to fix | ||
87 | 09:44 <+tmortagne> right now the velocity namespace is set for the page content rendering but should be set for the whole page display I think | ||
88 | 09:44 <+tmortagne> that's probably the issue | ||
89 | 09:51 <sburjan> has joined #xwiki | ||
90 | 10:32 <jvdrean> has joined #xwiki | ||
91 | 11:40 <vmassol> mflorea: I see you're modifying some app (like the blog app). What happens if someone updates to XE 3.2 final but doesn't update the blog app for ex. Will it continue to work? | ||
92 | 11:41 <vmassol> (want to be sure we're backward compatible at the page level) | ||
93 | 11:42 <+mflorea> yep, it should work fine. I update the blog (and I updating the administration app) just for the proof of concept | ||
94 | 11:43 <vmassol> ok good | ||
95 | 11:43 <vmassol> thanks | ||
96 | 11:46 <pturcotte> has quit | ||
97 | 12:11 <+tmortagne> sburjan: BTW I added some stuff in the demo UI of EM yesterday and this morning | ||
98 | 12:11 <+tmortagne> no new features, just exposing more information | ||
99 | 12:23 <jvdrean> has quit | ||
100 | 12:27 <@cjdelisle> restarting myxwiki to enable debug logging on the velocity engine to find where the macro is redefined. | ||
101 | 12:28 <vmassol> k | ||
102 | 12:33 <+sburjan> tmortagne: I will test it next week | ||
103 | 12:33 <+sburjan> guys, it seems we are getting spammed on the dev list | ||
104 | 12:33 <vmassol> sburjan: used to get spam I think | ||
105 | 12:33 <+sburjan> with forgery web sites | ||
106 | 12:33 <vmassol> hmm unless my spam filter is removing them | ||
107 | 12:34 <+sburjan> [email protected] | ||
108 | 12:34 <+sburjan> is the sender e-mail address | ||
109 | 12:34 <vmassol> yep I'ver emoved that user | ||
110 | 12:34 <vmassol> check the mails | ||
111 | 12:34 <vmassol> they must be old | ||
112 | 12:34 <+sburjan> 3 days ago | ||
113 | 12:34 <vmassol> yep | ||
114 | 12:34 <+sburjan> cool | ||
115 | 12:34 <vmassol> that's old :) | ||
116 | 12:38 <+sburjan> ;) | ||
117 | 12:44 <@cjdelisle> 2011-09-15 10:43:49,548 [http://evrueppurr.myxwiki.org/xwiki/bin/view/Blog/] DEBUG o.x.v.i.DefaultVelocityEngine - Velocimacro : added VM panelheader: source=/templates/macros.vm | ||
118 | 12:45 <@cjdelisle> foundit | ||
119 | 12:45 <vmassol> nice catch! | ||
120 | 12:45 <@cjdelisle> or maybe not, it says "added" not overridden | ||
121 | 12:46 <@cjdelisle> I'll look carefully at that subwiki anyway | ||
122 | 13:08 <@cjdelisle> http://xwikiday.myxwiki.org/ | ||
123 | 13:08 <@cjdelisle> nice web design | ||
124 | 13:08 <vmassol> yep that was a wiki done by XWiki SAS | ||
125 | 13:09 <vmassol> (not used anymore ATM) | ||
126 | 13:10 <@cjdelisle> I'm just loading all of the pages looking for anything else which reports overriding those macros | ||
127 | 13:10 <@cjdelisle> *each wiki | ||
128 | 13:25 <@cjdelisle> http://slussen.myxwiki.org/xwiki/bin/edit/Main/SlussenSkin?editor=object | ||
129 | 13:26 <@cjdelisle> bingo | ||
130 | 13:26 <@cjdelisle> 2011-09-15 11:23:27,092 [http://slussen.myxwiki.org/xwiki/bin/view/Start/WebHome] DEBUG o.x.v.i.DefaultVelocityEngine - Velocimacro : added VM panelheader: source= | ||
131 | 13:27 <vmassol> that rings a bell, I think I had found this wiki before | ||
132 | 13:27 <vmassol> sergiu looked at it AFAIR but said that wasn't the problem. Anyway I don't fully remember | ||
133 | 13:30 <jvdrean> has joined #xwiki | ||
134 | 13:36 <jvelociter> has joined #xwiki | ||
135 | 13:41 <@cjdelisle> yup, I'm 99% sure that's the one, it has the same onclick= that's showing up in the macro | ||
136 | 13:43 <vmassol> k cool | ||
137 | 13:45 <@cjdelisle> it looks like it would be pretty difficult work around it in their skin without breaking them but I'll see if I can repeat the problem and maybe we can get a quick fix rolled out. | ||
138 | 13:47 <vmassol> sounds good | ||
139 | 13:57 <+sburjan> maven.xwiki.org is down ? | ||
140 | 13:57 <+sburjan> no, just slow | ||
141 | 13:59 <@cjdelisle> XWIKI-6972 | ||
142 | 14:05 <@cjdelisle> restarting myxwiki with debug logging in velocity removed so we don'e end up with 10GB of logs | ||
143 | 14:18 <+mflorea> vmassol: I'm ready to merge the feature-sheets branch | ||
144 | 14:18 <vmassol> mflorea: ok... | ||
145 | 14:18 <DrLou> has joined #xwiki | ||
146 | 14:18 <+mflorea> let's see how it goes | ||
147 | 14:20 <@cjdelisle> I kind of feel like I need a new way of visualizing code to trace an execution path through oldcore | ||
148 | 14:25 <pturcotte> has joined #xwiki | ||
149 | 14:54 <@cjdelisle> return XWikiVelocityRenderer.evaluate(content, "", (VelocityContext) context.get("vcontext"), | ||
150 | 14:54 <@cjdelisle> (in parseTemplate(String template, String skin, XWikiContext context)) | ||
151 | 14:54 <@cjdelisle> "" <-- put in root namespace | ||
152 | 15:06 <sdumitriu> has joined #xwiki | ||
153 | 15:06 <@cjdelisle> sdumitriu: I think I found it: http://slussen.myxwiki.org/xwiki/bin/edit/Main/SlussenSkin?editor=object | ||
154 | 15:07 <@sdumitriu> Yep, I thought that too for a long time | ||
155 | 15:07 <@sdumitriu> But it's not that one | ||
156 | 15:08 <@sdumitriu> I mean it wasn't the one that was causing problems before | ||
157 | 15:08 <@cjdelisle> how do you know? | ||
158 | 15:08 <@sdumitriu> It does redefine the macros, true | ||
159 | 15:08 <@sdumitriu> So it is causing your problem | ||
160 | 15:08 <@sdumitriu> But there's another one that was messing things up even worse | ||
161 | 15:09 <@cjdelisle> XWIKI-6972 | ||
162 | 15:12 <@cjdelisle> https://github.com/xwiki/xwiki-platform/blob/master/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/XWiki.java#L1832 | ||
163 | 15:13 <@cjdelisle> if someone defines a skin document, that shouldn't be put in the root namespace correct? | ||
164 | 15:14 <@sdumitriu> I wonder what would break if we set a proper namespace there | ||
165 | 15:15 <@cjdelisle> I think the namespace should be: Wiki:Space.SkinName | ||
166 | 15:16 <@cjdelisle> Since header is parsed every time a page is loaded with a skin applied, it shouldn't be possible to break the header | ||
167 | 15:17 <@cjdelisle> I suppose there might be things which define macros in the footer and expect them to work above the footer and there's a possibility of them breaking but is that going to be very common? | ||
168 | 15:20 <@cjdelisle> I guess I have to look more closely at how velocity handles namespaces | ||
169 | 16:38 <+mflorea> tmortagne: do you know why http://ci.xwiki.org/job/xwiki-platform/org.xwiki.platform$xwiki-platform-extension-repository-aether/630/testReport/org.xwiki.extension.repository.aether.internal/AetherDefaultRepositoryManagerTest/testResolve/ is failing? | ||
170 | 16:38 <+tmortagne> mflorea: sounds like a missing push :) | ||
171 | 16:38 <+tmortagne> it's the last build ? | ||
172 | 16:38 <+tmortagne> checking | ||
173 | 16:39 <+mflorea> it is failing for the past 4 builds | ||
174 | 16:39 <+mflorea> I hope it's not related to my branch merge | ||
175 | 16:40 <+tmortagne> ha no looks like a fixed one test and not the other | ||
176 | 16:40 <+tmortagne> it's not your fault for sure ;) | ||
177 | 16:40 <+mflorea> good | ||
178 | 16:40 <+mflorea> :) | ||
179 | 16:41 <+mflorea> I'm waiting to see if there are any functional tests failing after the merge, but I need platform to build first | ||
180 | 16:43 <+tmortagne> mflorea: should be ok now | ||
181 | 16:43 <+mflorea> great, thanks | ||
182 | 16:44 <+tmortagne> ha there is not only failing tests | ||
183 | 16:44 <+tmortagne> Caused by: org.sonatype.aether.transfer.ArtifactNotFoundException: Could not find artifact org.xwiki.platform:xwiki-platform-display:jar:3.2-SNAPSHOT in xwiki-snapshots (http://nexus.xwiki.org/nexus/content/groups/public-snapshots) | ||
184 | 16:44 <+tmortagne> :) | ||
185 | 16:45 <+mflorea> nexus cache issue? | ||
186 | 16:45 <+mflorea> no :) my fault | ||
187 | 16:45 <+mflorea> fixing it | ||
188 | 16:46 <+mflorea> n00b mistake | ||
189 | 16:46 <@sdumitriu> Forgot to add it as a submodule to the platform pom? | ||
190 | 16:46 <+mflorea> yep | ||
191 | 16:46 <+mflorea> :) | ||
192 | 17:01 <vmassol> sdumitriu: I'd like to start creating a feature branch in platform for the new model and thus move the model I've put in xwiki-contrib/sandbox into my platform branch. I was thinking that preserving history would be nice. Could I use your script to do that? | ||
193 | 17:02 <@sdumitriu> Yes | ||
194 | 17:02 <@sdumitriu> Let me clean it up | ||
195 | 17:04 <vmassol> ok thanks | ||
196 | 17:04 <vmassol> guys do we still need the "remotes/origin/enabled-csrf-protection" branch? | ||
197 | 17:05 <@sdumitriu> Probably not, it was merged IIRC | ||
198 | 17:05 <@sdumitriu> Checking | ||
199 | 17:05 <vmassol> ah yes github compare | ||
200 | 17:06 <@sdumitriu> Yep, all merged | ||
201 | 17:06 <@sdumitriu> I did git cherry HEAD refs/remotes/origin/enabled-csrf-protection | ||
202 | 17:07 <vmassol> ah nice didn't know about this | ||
203 | 17:33 <@sdumitriu> vmassol: Where do you want the module to appear after extracting it? Root of the repo, /xwiki-model/, xwiki-platform-core/xwiki-platform-model/, something else? | ||
204 | 17:34 <+jvdrean> Hi, do we have know issues with $xwiki.copyDocument in 3.1 ? | ||
205 | 17:41 <vmassol> sdumitriu: xwiki-platform-core/xwiki-platform-model/ | ||
206 | 17:42 <@sdumitriu> This one already exists | ||
207 | 17:42 <vmassol> yes | ||
208 | 17:42 <@sdumitriu> You will get conflicts when mergin, I think | ||
209 | 17:42 <vmassol> not really | ||
210 | 17:42 <@sdumitriu> OK | ||
211 | 17:42 <vmassol> because in sandbox they're in differnet modules | ||
212 | 17:42 <vmassol> I'll need to do the merge locally anywya but it'll be a simpl eone | ||
213 | 17:43 <vmassol> sdumitriu: I want to merge this in my branch, not on master (just making sure) | ||
214 | 17:43 <@sdumitriu> OK | ||
215 | 17:44 <@sdumitriu> Sending you the script after I check it works | ||
216 | 17:44 <vmassol> k thanks | ||
217 | 17:59 <tmortagne> has quit | ||
218 | 18:01 <@sdumitriu> vmassol: Sent | ||
219 | 18:02 <@sdumitriu> Report any problems you have | ||
220 | 18:44 <vmassol> sdumitriu: thanks | ||
221 | 18:45 <@sdumitriu> Works? | ||
222 | 18:45 <vmassol> was away haven't tried yet | ||
223 | 18:45 <vmassol> will let you know | ||
224 | 18:47 <vmassol> sburjan: fyi XE-1016 | ||
225 | 18:49 <+sburjan> looks nice | ||
226 | 18:49 <+sburjan> Improvements in the mouse emulation (movement, click, double click, context | ||
227 | 18:49 <+sburjan> click) | ||
228 | 18:51 <+sburjan> this should be nice, but I don't know if select text within a container WebElement is implemented | ||
229 | 19:34 <sburjan> has quit | ||
230 | 19:42 <vmassol> has quit | ||
231 | 20:35 <lpereira> has joined #xwiki | ||
232 | 20:58 <DrLou> has quit | ||
233 | 21:01 <vmassol> has joined #xwiki | ||
234 | 21:04 <DrLou> has joined #xwiki | ||
235 | 21:27 <vmassol> mflorea: hi | ||
236 | 21:27 <vmassol> I see you've used this for creating a logger: | ||
237 | 21:27 <vmassol> private static final Logger LOGGER = LoggerFactory.getLogger(ConfiguredDocumentDisplayer.class); | ||
238 | 21:27 <+mflorea> yes | ||
239 | 21:27 <vmassol> but the best way is: | ||
240 | 21:27 <vmassol> @Inject Logger logger; | ||
241 | 21:27 <vmassol> s/best/canonical | ||
242 | 21:28 <+mflorea> nice.. | ||
243 | 21:28 <+mflorea> wasn't aware of this | ||
244 | 21:28 <+mflorea> I'll update my code | ||
245 | 21:28 <vmassol> ok cool | ||
246 | 21:28 <vmassol> btw I've added missing @Singleton | ||
247 | 21:28 <+mflorea> thanks | ||
248 | 21:29 <vmassol> @Inject private Logger logger; | ||
249 | 21:29 <vmassol> (missed "private" above) | ||
250 | 21:29 <+mflorea> right | ||
251 | 21:33 <vmassol> mflorea: another point | ||
252 | 21:33 <vmassol> String format = "Failed to lookup document displayer with hint [%s]. Using default document displayer."; | ||
253 | 21:33 <vmassol> LOGGER.warn(String.format(format, documentDisplayerHint)); | ||
254 | 21:34 <vmassol> the best practice is to use: | ||
255 | 21:34 <vmassol> this.logger.warn("Failed to lookup document displayer with hint {}. Using default document displayer.", documentDisplayerHint); | ||
256 | 21:35 <+mflorea> nice, but what if I want to also log the exception? | ||
257 | 21:35 <+mflorea> ie. warn(String, Throwable) | ||
258 | 21:36 <+mflorea> but maybe that is not needed | ||
259 | 21:36 <vmassol> well in this example you're not logging the exception | ||
260 | 21:36 <+mflorea> sure | ||
261 | 21:36 <vmassol> now they're missing an api | ||
262 | 21:36 <vmassol> AFAIK | ||
263 | 21:36 <vmassol> I'm nt sure why | ||
264 | 21:36 <+mflorea> I'm wondering about the cases when the exception is also logged | ||
265 | 21:36 <vmassol> maybe they support it | ||
266 | 21:37 <vmassol> maybe http://www.slf4j.org/apidocs/org/slf4j/Logger.html#warn(java.lang.String, java.lang.Object, java.lang.Object) | ||
267 | 21:37 <vmassol> maybe the first or second param can be an exception | ||
268 | 21:37 <vmassol> I need to google a bit | ||
269 | 21:37 <vmassol> I don't know the answer | ||
270 | 21:37 <+mflorea> anyway, it's not that important to log the exception for a warning | ||
271 | 21:38 <vmassol> found it | ||
272 | 21:38 <vmassol> http://www.slf4j.org/faq.html#paramException | ||
273 | 21:38 <vmassol> as the last param | ||
274 | 21:38 <vmassol> :) | ||
275 | 21:38 <+mflorea> :) nice | ||
276 | 21:39 <+mflorea> I'll update the code | ||
277 | 21:39 <+mflorea> thanks | ||
278 | 21:39 <vmassol> but yes | ||
279 | 21:39 <vmassol> we shouldn't print stack trace with warnings | ||
280 | 21:39 <vmassol> only with errors | ||
281 | 21:42 <vmassol> btw technically a displayer doesn't display btw ;) | ||
282 | 21:43 <vmassol> they seem to generate XDOM | ||
283 | 21:43 <+mflorea> yes, I followed Thomas's suggestion | ||
284 | 21:43 <vmassol> the renderer renders and the template display | ||
285 | 21:43 <vmassol> ;) | ||
286 | 21:44 <vmassol> generating XDOM is very good. I was just commenting on the naming | ||
287 | 21:44 <+mflorea> well, the "displayer" name was taken from http://dev.xwiki.org/xwiki/bin/view/Design/FrontEndArchitecture | ||
288 | 21:45 <vmassol> yep | ||
289 | 21:45 <vmassol> I know | ||
290 | 21:46 <vmassol> I'm really happy that we've started this module at last! :) | ||
291 | 21:46 <+mflorea> :) | ||
292 | 21:49 <vmassol> hmmm seems you're executing content to get the title in extractTitleFromContent | ||
293 | 21:50 <vmassol> in DocumentTitleDisplayer | ||
294 | 21:50 <vmassol> AFAIR we had decided not to do this in the past | ||
295 | 21:50 <+mflorea> yes, that was the behavior in XWikiDocument | ||
296 | 21:50 <vmassol> hmm I don't remember this | ||
297 | 21:50 <vmassol> I remember the opposite | ||
298 | 21:51 <+mflorea> nop, I'm pretty sure | ||
299 | 21:51 <vmassol> becasue we said it was too dangerous | ||
300 | 21:51 <vmassol> (and not performant) | ||
301 | 21:51 <vmassol> let me check | ||
302 | 21:51 <vmassol> maybe I'm mixing with another getTitle api | ||
303 | 21:53 <+mflorea> https://github.com/xwiki/xwiki-platform/commit/aca51893724cb5203e5c7f5cbecdd62d91ec0a1c#diff-14 | ||
304 | 21:53 <+mflorea> look for getRenderedContentTitle | ||
305 | 21:54 <vmassol> I'm probably confused. I'm pretty sure I remmeber that a document with only an include in it will not get its title extracted | ||
306 | 21:54 <vmassol> oh I know | ||
307 | 21:54 <vmassol> it's only rendering the header | ||
308 | 21:54 <+mflorea> yes | ||
309 | 21:55 <vmassol> ok my bad, that's ok | ||
310 | 21:55 <vmassol> btw you don't have unit tests for the new modules it seems | ||
311 | 21:55 <vmassol> I don't see any in display module | ||
312 | 21:55 <+mflorea> yes.. | ||
313 | 21:55 <vmassol> (which means they were not developed TDD… bad bad ;)) | ||
314 | 21:56 <vmassol> we really need 80-90% coverage for all new code | ||
315 | 21:56 <vmassol> I wonder how you've tested them, it's so much easier with unit tests | ||
316 | 21:56 <vmassol> anyway please add them when you can | ||
317 | 21:57 <+mflorea> I know.. I was saved by the unit tests in old core and include macro | ||
318 | 21:57 <+mflorea> I will | ||
319 | 21:58 <vmassol> I like the fact that each class doesn't have much code, this tends to mean it has a good design | ||
320 | 21:58 <vmassol> I think there are some fine tuning to do on the exception handling part | ||
321 | 22:00 <+mflorea> I had to split the code as mush as possible because I was getting cyclomatic complexity very easy (too many imports) | ||
322 | 22:01 <+mflorea> yes, exception handling needs to be improved | ||
323 | 22:05 <finbrein> has quit | ||
324 | 22:07 <vmassol> mflorea: do you know why we need a displayer in the include macro when context is current? | ||
325 | 22:07 <vmassol> I was expecting the displayer to be used only when context is NEW | ||
326 | 22:08 <+mflorea> just to prevent duplicating the getContent method, especially when the section is specified | ||
327 | 22:09 <+mflorea> and for consistency | ||
328 | 22:09 <vmassol> I need to think a bit more about this but there are some things that we need to fix in the platform rendering | ||
329 | 22:09 <+mflorea> displayParameters.setContentTransformed(false) => the displayer does nothing more than to retrieve the content | ||
330 | 22:10 <vmassol> we need to move the include macro in the rendering module in the future | ||
331 | 22:11 <vmassol> IMO we need some way to return a XDOM without having to perform any rendering in the macro itself | ||
332 | 22:11 <vmassol> even when context = NEW | ||
333 | 22:12 <vmassol> i need to brainstorm a bit with thomas about this | ||
334 | 22:12 <vmassol> but this would allow to more cleanly separate rendering from the rest | ||
335 | 22:12 <vmassol> (rest = other platform modules) | ||
336 | 22:13 <vmassol> I'll let you know if I come up with something | ||
337 | 22:13 <+mflorea> I guess you need a way to mark that a subtree of XDOM is transformed in new context | ||
338 | 22:13 <vmassol> yes | ||
339 | 22:13 <vmassol> exatly | ||
340 | 22:13 <vmassol> exactly | ||
341 | 22:15 <vmassol> exception handling is a btit strange here: | ||
342 | 22:15 <vmassol> try { | ||
343 | 22:15 <vmassol> result = documentDisplayer.display(documentBridge, displayParameters); | ||
344 | 22:15 <vmassol> } catch (Exception e) { | ||
345 | 22:15 <vmassol> throw new MacroExecutionException(e.getMessage(), e); | ||
346 | 22:15 <vmassol> } | ||
347 | 22:15 <vmassol> display doesn't thrown an excpetion and yet we expect one, which tends to mean mayb dsplay should thrown an excpetion | ||
348 | 22:15 <+mflorea> :) that's actually a hack to make a test of include macro pass | ||
349 | 22:16 <vmassol> (not sure why it doesn't thwo one) | ||
350 | 22:16 <vmassol> Macro and Transformations do throw an exception | ||
351 | 22:17 <vmassol> and it seems to me it's possible to get all sort of problems when calling a displayer | ||
352 | 22:17 <+mflorea> indeed | ||
353 | 22:17 <vmassol> I see a lot of throw RuntimeException in displayer | ||
354 | 22:17 <+mflorea> yep | ||
355 | 22:17 <vmassol> IMO the problem is that display() methjod doesn't throw a DisplayException | ||
356 | 22:17 <+mflorea> that was the quick solution | ||
357 | 22:18 <+mflorea> indeed, an expected DisplayException in the API is better | ||
358 | 22:19 <vmassol> would be good that we stabilize the api as much as possibe before the final 3.2 release | ||
359 | 22:19 <vmassol> I'll continue to review the committed code tomorrow | ||
360 | 22:21 <vmassol> lots of failing functional tests remain apparently | ||
361 | 22:26 <+mflorea> hmm, they weren't failing just after I merged the branch | ||
362 | 22:26 <vmassol> the webstandarsd one seem to be caused by a jvm crash | ||
363 | 22:26 <+mflorea> I need to check them | ||
364 | 22:26 <vmassol> I'm restartgin them now | ||
365 | 22:27 <vmassol> I also updated selenium to 2.6.0 | ||
366 | 22:27 <vmassol> but I ran it locally first | ||
367 | 22:27 <vmassol> and all tests were passing locally | ||
368 | 22:27 <vmassol> (except the famous one that doesn't pass on my mac) | ||
369 | 22:27 <+mflorea> :) | ||
370 | 22:28 <+mflorea> so it still fails with 2.6.0, I need to check if it fail without my hack | ||
371 | 22:28 <vmassol> indeed | ||
372 | 22:29 <finbrein> has joined #xwiki | ||
373 | 22:37 <mflorea> has quit | ||
374 | 22:43 <vmassol> ok only 1 error in webstandard now | ||
375 | 22:43 <vmassol> http://ci.xwiki.org/job/xwiki-enterprise-test-webstandards/org.xwiki.enterprise$xwiki-enterprise-test-webstandards/281/testReport/org.xwiki.test.webstandards/RSSValidationTest/Validating_RSS_validity_for____xwiki__bin__view__Main__LuceneSearch_xpage_plain_outputSyntax_plain_text_wiki_executed_as_guest/ | ||
376 | 22:43 <vmassol> got to go, bed time | ||
377 | 22:44 <vmassol> has quit | ||
378 | 22:55 <finbrein> has quit | ||
379 | 22:56 <finbrein> has joined #xwiki | ||
380 | 23:10 <pturcotte> has quit | ||
381 | 23:14 <jvdrean> has quit |