1 2014-05-27T00:25:34 *** magu_cic
2 2014-05-27T00:29:02 *** magu_cic_
3 2014-05-27T01:24:21 *** derdon
4 2014-05-27T01:25:50 *** derdon
5 2014-05-27T01:32:01 *** derdon
6 2014-05-27T03:06:49 *** sl33k_
7 2014-05-27T03:15:46 *** sl33k_
8 2014-05-27T06:44:03 *** randomax
9 2014-05-27T06:49:25 *** randomax
10 2014-05-27T07:30:10 *** skathpalia
11 2014-05-27T07:30:15 <skathpalia> moin
12 2014-05-27T07:31:12 <skathpalia> ThomasWaldmann, dimazest I have made the changes in my cr as according to the discussion we had https://codereview.appspot.com/100720043
13 2014-05-27T07:31:26 <skathpalia> Can you please review it
14 2014-05-27T07:54:07 <dimazest> morning
15 2014-05-27T07:54:14 <dimazest> please see my comments
16 2014-05-27T08:02:52 *** randomax
17 2014-05-27T08:10:03 <randomax> moin
18 2014-05-27T08:10:13 <randomax> ThomasWaldmann, code review needed https://codereview.appspot.com/91680043/
19 2014-05-27T08:15:49 *** sl33k_
20 2014-05-27T08:36:43 <skathpalia> dimazest, I have updated updated the cr
21 2014-05-27T08:41:01 <skathpalia> dimazest, I could have used directly choices = [(rev.meta[ITEMID], rev.meta[cls.properties['label_meta_key']]) for rev in revs] but in case of names it is a list for which only first name needs to be shown and in case of ITEMID it is just a string
22 2014-05-27T08:53:14 <dimazest> yes, but i would define a function which handles this logic
23 2014-05-27T08:53:33 <dimazest> and keep just one list comprehension
24 2014-05-27T08:53:56 * dimazest is on his way to university
25 2014-05-27T08:54:14 *** magu_cic_
26 2014-05-27T08:55:02 <skathpalia> dimazest, may be we can replace elif with else
27 2014-05-27T08:55:31 <skathpalia> as there is name only which is a list others are not
28 2014-05-27T08:56:36 <skathpalia> What do you think about this?
29 2014-05-27T08:57:50 *** magu_cic
30 2014-05-27T09:05:38 *** greg_f
31 2014-05-27T10:17:41 <dimazest> that's better
32 2014-05-27T10:17:53 <dimazest> but i'm still for having only one list comprehension
33 2014-05-27T10:18:03 <dimazest> to avoid code duplication
34 2014-05-27T10:23:01 <skathpalia> dimazest, Can you suggest a way to have only one?
35 2014-05-27T10:23:26 <dimazest> def get_value():
36 2014-05-27T10:23:38 <dimazest> wait...
37 2014-05-27T10:24:06 <dimazest> def get_label(rev):
38 2014-05-27T10:25:14 <dimazest> value = rev.meta[cls.properties['label_meta_key']]
39 2014-05-27T10:25:37 <dimazest> return value[0] if cls.properties['label_meta_key' == NAME else value
40 2014-05-27T10:25:53 <dimazest> or
41 2014-05-27T10:26:27 <dimazest> some nicer logic that decide whether value has to be unpacked
42 2014-05-27T10:27:01 <dimazest> maybe checking whether it's a list or not
43 2014-05-27T10:27:08 <skathpalia> Okay I will try to find some better logic otherwise I will define this function only
44 2014-05-27T10:29:33 <skathpalia> dimazest, I think we should discuss this with ThomasWaldmann and get his views also on this
45 2014-05-27T10:29:42 <skathpalia> May be he has some better idea
46 2014-05-27T10:30:20 <dimazest> sure
47 2014-05-27T10:32:36 <skathpalia> dimazest, I also worked on some UI part of tickets http://picpaste.com/pics/Screenshot_from_2014-05-27_14_31_57-MoJplfdc.1401186715.png
48 2014-05-27T10:32:48 <skathpalia> please have a looka at it
49 2014-05-27T10:33:23 <skathpalia> In this I defined css for submit buttons, text input and radio buttons
50 2014-05-27T10:33:31 <dimazest> looks nice
51 2014-05-27T10:33:43 <skathpalia> I also increased the padding which was pointed by ThomasWaldmann
52 2014-05-27T10:34:56 <skathpalia> Thanks :)
53 2014-05-27T10:35:13 <skathpalia> I will soon define the css for the select boxes also
54 2014-05-27T10:52:55 *** skathpalia
55 2014-05-27T11:43:40 *** sl33k_
56 2014-05-27T12:10:28 *** magu_cic_
57 2014-05-27T13:28:07 *** sl33k_
58 2014-05-27T13:37:31 *** skathpalia
59 2014-05-27T14:20:21 *** RogerHaase
60 2014-05-27T14:23:33 <skathpalia> RogerHaase, we had a discussion on having stylus for defining css or use it directly from ticket.css
61 2014-05-27T14:23:54 <skathpalia> What do you suggest?
62 2014-05-27T14:24:09 <skathpalia> I mean what should we use?
63 2014-05-27T14:31:00 <RogerHaase> skathpalia: I think straight css for now. imho stylus has been buggy and over long term I would switch modernized theme to bootstrap (but I do not yet have enough experience with bootstrap, maybe we will hit bugs there too)
64 2014-05-27T14:31:24 <RogerHaase> tw may have different opinion
65 2014-05-27T14:32:25 <skathpalia> Okay I will ask ThomasWaldmann also about this
66 2014-05-27T14:39:04 <randomax> RogerHaase, code review needed https://codereview.appspot.com/91680043/
67 2014-05-27T14:40:58 *** sl33k_
68 2014-05-27T14:44:44 <RogerHaase> randomax: looking; btw you lucked out this time, but in general you cannot do a pull on your repo in the middle of a code review. If a changeset you pull updates the same files as your CR, then all the pulled changes will be reflected in the code review, very confusing...
69 2014-05-27T14:51:58 <RogerHaase> randomax: there is still a problem with long item names. Create an item with a long name, add a quicklink to that item, then try to delete the quicklink in the basic theme. -- clicking the X creates another quicklink to same page
70 2014-05-27T14:54:12 *** magu_cic
71 2014-05-27T15:01:19 <RogerHaase> randomax: seems like global index gets messed up as well, there a 2 entries for the item with the long name. I suggest you commit and push the current cs and do the Basic-theme-long-item-name problem as a separate cs
72 2014-05-27T15:07:05 <randomax> RogerHaase, ok... i think there is a problem in the href of the links.. they are getting shortened urls.. i'll work on it..
73 2014-05-27T15:07:17 <RogerHaase> randomax: updated https://codereview.appspot.com/91680043/ with above comments
74 2014-05-27T15:08:42 *** dave_largo
75 2014-05-27T15:11:10 <ThomasWaldmann> randomax: do not ask for more feedback unless you reacted to previous feedback
76 2014-05-27T15:11:39 <ThomasWaldmann> skathpalia: see CR
77 2014-05-27T15:13:57 <ThomasWaldmann> skathpalia: ah, you already discussed it with dimazest
78 2014-05-27T15:14:36 <dimazest> yes, but we decided to wait for your comments
79 2014-05-27T15:14:50 <ThomasWaldmann> i agree, using a getter function is the way
80 2014-05-27T15:15:43 <ThomasWaldmann> but do not repeat stuff like cls.properties['label_meta_key'] multiple times, just assign that to "key" once and then use key
81 2014-05-27T15:15:55 <randomax> ThomasWaldmann, sorry.. will keep that in mind the next time
82 2014-05-27T15:16:32 <ThomasWaldmann> also, do not use key == NAME to decide, but rather key in [NAME, ]
83 2014-05-27T15:16:34 *** penguinRaider
84 2014-05-27T15:17:19 <ThomasWaldmann> (as there might be other meta items also resulting in a list, so they should receive same treatment as NAME key)
85 2014-05-27T15:19:58 <skathpalia> ThomasWaldmann, I was thinking of having else statement rather that elif
86 2014-05-27T15:20:16 <skathpalia> as only names are list
87 2014-05-27T15:20:44 <ThomasWaldmann> tags are also a list
88 2014-05-27T15:21:37 <RogerHaase> randomax: also look at possibility of both js events firing - clicking on the X triggers the event to delete the quicklink, and another to go to the item with the long name. The X js event must stop event bubbling
89 2014-05-27T15:22:19 <skathpalia> Then I think I have to do something like this if key in [NAME, TAGS, ...]
90 2014-05-27T15:22:53 <skathpalia> I mean take all those which are in the form of list in the if statement
91 2014-05-27T15:30:38 <skathpalia> ThomasWaldmann, I couldn't see the key defined for tags for ticket items
92 2014-05-27T15:30:44 <skathpalia> in keys.y
93 2014-05-27T15:30:49 <skathpalia> *keys.py
94 2014-05-27T15:31:49 <skathpalia> There is a TAGS key but I am not sure that it is also used for ticket items
95 2014-05-27T15:32:50 <skathpalia> Can you please see to it?
96 2014-05-27T15:37:19 <skathpalia> Oh I think I got it
97 2014-05-27T15:40:47 <skathpalia> ThomasWaldmann, I have considered these which can take multiple values [NAME, TAGS, ITEMLINKS, ITEMTRANSCLUSIONS]
98 2014-05-27T15:41:19 <skathpalia> I think these are the only ones
99 2014-05-27T15:41:53 <skathpalia> right?
100 2014-05-27T15:46:00 <ThomasWaldmann> skathpalia: TAGS is a generic feature possible for any kind of item
101 2014-05-27T15:46:25 <skathpalia> sorry I was thinking something different
102 2014-05-27T15:46:58 <ThomasWaldmann> but I suggest you only list the ones at that place in the code that somehow make sense
103 2014-05-27T15:47:00 <dimazest> what if we get the value and check, if it's a list, then the first element is returned, otherwise the value is returned
104 2014-05-27T15:47:45 <skathpalia> ThomasWaldmann, I have updated the cr
105 2014-05-27T15:47:58 <skathpalia> Can you please have a look at it https://codereview.appspot.com/100720043
106 2014-05-27T15:48:43 <ThomasWaldmann> dimazest: that's also an option. in general somehow this "picking the first" is a bit strange, as the first might be as good as any other (or not).
107 2014-05-27T15:50:18 <ThomasWaldmann> skathpalia: you didn't make the getter function
108 2014-05-27T15:50:30 <skathpalia> Oh sorry I forgot to make that
109 2014-05-27T15:50:34 <dimazest> would it make sense to: from MoinMoin.constants import keys
110 2014-05-27T15:50:34 <ThomasWaldmann> thus you still have the list comprehension twice
111 2014-05-27T15:51:19 <skathpalia> making the function now
112 2014-05-27T15:51:26 <ThomasWaldmann> dimazest: less fluctuations in the imports, yes, but otoh, more to write when using it...
113 2014-05-27T15:52:15 <skathpalia> Then I think I have to run one for loop and then check for the key
114 2014-05-27T15:52:20 <skathpalia> right?
115 2014-05-27T15:54:08 <ThomasWaldmann> you can either have the current if/else and define 2 different, but same-name getter functions.
116 2014-05-27T15:54:39 <ThomasWaldmann> or you can define 1 getter fn and do a type check inside (to determine whether you got a list)
117 2014-05-27T15:56:52 <ThomasWaldmann> but in any case: do not list ITEMLINKS, ITEMTRANSCLUSIONS, TAGS there. they are list, but it does not make sense to have them there.
118 2014-05-27T15:58:02 *** magu_cic_
119 2014-05-27T15:58:17 <skathpalia> I am thinking of having this if/else statements and have a function get_choices(revs, key) which returns the choices
120 2014-05-27T15:59:45 <ThomasWaldmann> that's maybe not better than what you have right now (if i understand you correctly)
121 2014-05-27T16:00:29 <ThomasWaldmann> but maybe think about that for a while on your own
122 2014-05-27T16:01:29 *** magu_cic
123 2014-05-27T16:03:31 *** magu_cic_
124 2014-05-27T16:12:35 <skathpalia> ThomasWaldmann, I have made a getter function
125 2014-05-27T16:12:40 *** magu_cic
126 2014-05-27T16:12:51 <skathpalia> Please have a look at it http://codereview.appspot.com/100720043
127 2014-05-27T16:14:38 <dimazest> no, this in not a very nice solution
128 2014-05-27T16:14:49 <dimazest> actually, this is a python antipattern
129 2014-05-27T16:15:23 <dimazest> inside of def _get_choice_specs(cls)
130 2014-05-27T16:15:24 *** jhermann
131 2014-05-27T16:15:29 <dimazest> you can define a function
132 2014-05-27T16:15:42 <dimazest> def get_label(rev):
133 2014-05-27T16:15:53 <skathpalia> First I thought of that only :(
134 2014-05-27T16:16:56 <skathpalia> Now I am defining that function inside only
135 2014-05-27T16:17:11 <dimazest> you also should use .isinstance(rev.meta[key], list)
136 2014-05-27T16:23:29 <skathpalia> dimazest, ThomasWaldmann I have updated th cr
137 2014-05-27T16:26:04 <dimazest> the function should take a revision
138 2014-05-27T16:26:07 <dimazest> not a list of revisions
139 2014-05-27T16:27:13 <skathpalia> Okay making it that way
140 2014-05-27T16:30:06 <skathpalia> dimazest, updated the cr
141 2014-05-27T16:30:19 <skathpalia> Now function takes only one revision
142 2014-05-27T16:34:44 <dimazest> see my comments
143 2014-05-27T16:39:24 <skathpalia> dimazest, changed the function to return label and also the name of the function
144 2014-05-27T16:41:00 <dimazest> still, you don't need to pass key to that function
145 2014-05-27T16:41:15 <dimazest> because it has access to the variable of the outer function
146 2014-05-27T16:42:16 <skathpalia> yeah my mistake
147 2014-05-27T16:42:21 <skathpalia> I have corrected that
148 2014-05-27T16:43:22 <dimazest> i would also put empty lines before and after the functin
149 2014-05-27T16:43:26 <dimazest> *function
150 2014-05-27T16:43:33 <dimazest> otherwise the code is too dense
151 2014-05-27T16:43:38 <skathpalia> Okay doing that
152 2014-05-27T16:44:11 <skathpalia> done that
153 2014-05-27T16:45:31 <dimazest> looks good!
154 2014-05-27T16:45:44 <dimazest> after all the changes and discussions :)
155 2014-05-27T16:45:58 <skathpalia> Yeah finally
156 2014-05-27T16:46:03 <skathpalia> Can i commit it now to my repo?
157 2014-05-27T16:47:58 <skathpalia> dimazest ?
158 2014-05-27T16:48:19 <dimazest> yes, i've been rechecking the code
159 2014-05-27T16:49:06 <skathpalia> Okay soon I will be sending the pull request :)
160 2014-05-27T16:53:42 <ThomasWaldmann> skathpalia: ok, looks good now
161 2014-05-27T16:53:55 <skathpalia> Thanks :)
162 2014-05-27T16:54:06 <skathpalia> I am making pr
163 2014-05-27T16:55:42 *** derdon
164 2014-05-27T16:57:33 <ThomasWaldmann> skathpalia: i am missing a change at the place where it crashed.
165 2014-05-27T16:58:37 <skathpalia> Earlier it was crashing at fetching names
166 2014-05-27T16:58:48 <skathpalia> I have changed that part only
167 2014-05-27T16:59:45 <ThomasWaldmann> hmm, maybe i am confusing it. i put an assert + comment at some place to make it crash early if there is no name
168 2014-05-27T16:59:53 <ThomasWaldmann> but that was for notification code ...
169 2014-05-27T17:00:15 <skathpalia> Yeah I would correct that soon
170 2014-05-27T17:00:30 <ThomasWaldmann> BUT
171 2014-05-27T17:00:44 <ThomasWaldmann> is the result of what you did there USABLE?
172 2014-05-27T17:00:53 <skathpalia> Now on ticket updation that asserion error is raised as the notification module demands name which is not there in ticket items
173 2014-05-27T17:01:20 <skathpalia> But still the ticket gets successfully updated
174 2014-05-27T17:02:12 <ThomasWaldmann> it now shows ITEMID as label
175 2014-05-27T17:02:16 <skathpalia> I changed the code such that it will look for ITEMIDs in case of tickets
176 2014-05-27T17:02:28 <skathpalia> Yeah it shows ITEMID as label
177 2014-05-27T17:02:33 <ThomasWaldmann> so, how is that usable? :)
178 2014-05-27T17:03:02 <skathpalia> I was thinking of making that shortening in the later commit
179 2014-05-27T17:03:30 <ThomasWaldmann> i am not just talking about shortening. but ok, do it in a separate commit.
180 2014-05-27T17:03:40 <skathpalia> Yeah sure
181 2014-05-27T17:04:42 * ThomasWaldmann feels like this is not the final solution :)
182 2014-05-27T17:05:19 <skathpalia> Yeah I have to work on the notification module to make it usable
183 2014-05-27T17:05:35 <skathpalia> I mean making the notification module independent of name
184 2014-05-27T17:05:59 <ThomasWaldmann> in the tickets, there is something like a short description, right. what is the meta key of that?
185 2014-05-27T17:06:24 <skathpalia> iirc it is summary
186 2014-05-27T17:08:20 <ThomasWaldmann> ok, so shouldn' the label be: "#%s %s" % (itemid[:4], summary[:50]) ?
187 2014-05-27T17:09:03 *** magu_cic
188 2014-05-27T17:09:14 <skathpalia> Can I add it to my next commit as I have already committed it?
189 2014-05-27T17:09:36 <ThomasWaldmann> sure. think about what that would mean to your getter fn
190 2014-05-27T17:10:09 <ThomasWaldmann> dimazest: ^
191 2014-05-27T17:10:28 *** magu_cic
192 2014-05-27T17:11:06 <skathpalia> ThomasWaldmann made the pr
193 2014-05-27T17:11:25 <skathpalia> I also have one more cr pending that solves #424 https://codereview.appspot.com/80700049/#ps20001
194 2014-05-27T17:11:27 <dimazest> makes sense
195 2014-05-27T17:11:37 <ThomasWaldmann> did you pull from main repo / merge first?
196 2014-05-27T17:13:03 <skathpalia> Yeah I pulled from my repo first
197 2014-05-27T17:13:27 <ThomasWaldmann> main repo is mine :)
198 2014-05-27T17:13:53 <skathpalia> I mean I pulled the changes from your repo
199 2014-05-27T17:14:07 <ThomasWaldmann> ok
200 2014-05-27T17:14:27 <ThomasWaldmann> ok, i suggest to finish the getter stuff now
201 2014-05-27T17:14:50 <skathpalia> Okay I am adding the summary part
202 2014-05-27T17:15:02 <ThomasWaldmann> any idea what it means for the getter fn?
203 2014-05-27T17:15:16 <RogerHaase> ThomasWaldmann: please close #7 and #182
204 2014-05-27T17:17:13 <RogerHaase> ThomasWaldmann: media support https://codereview.appspot.com/100760044/
205 2014-05-27T17:18:26 <skathpalia> the getter function should check if it key is ITEMID and then accordingly return summary+ITEMID
206 2014-05-27T17:18:47 <ThomasWaldmann> no
207 2014-05-27T17:19:13 <ThomasWaldmann> (well, it would work, but there's an easier way)
208 2014-05-27T17:20:29 <ThomasWaldmann> easier and much more flexible == win!
209 2014-05-27T17:24:43 <skathpalia> may be change the label_meta_key to have both summary and ITEMID
210 2014-05-27T17:25:18 <skathpalia> ThomasWaldmann, right?
211 2014-05-27T17:25:44 <ThomasWaldmann> you're getting nearer, but you're not there yet
212 2014-05-27T17:26:59 <RogerHaase> randomax: nice blog update
213 2014-05-27T17:27:20 <randomax> RogerHaase, thanx :)
214 2014-05-27T17:27:23 <ThomasWaldmann> skathpalia: you know that functions can be given as parameter also?
215 2014-05-27T17:27:43 <skathpalia> Yeah
216 2014-05-27T17:28:23 <ThomasWaldmann> so as we've seen that giving the label_meta_key is not enough to get usable behaviour, why not give the whole getter function?
217 2014-05-27T17:29:52 <ThomasWaldmann> (that also makes the fn easier as we will know what type we get)
218 2014-05-27T17:33:08 <skathpalia> I am not able to fully understand your method but I think I should give some more time to understand it
219 2014-05-27T17:37:29 *** skathpalia
220 2014-05-27T17:44:31 *** magu_cic_
221 2014-05-27T17:47:41 *** magu_cic
222 2014-05-27T17:48:32 *** randomax
223 2014-05-27T17:54:13 *** magu_cic
224 2014-05-27T17:58:01 *** magu_cic_
225 2014-05-27T18:05:08 *** greg_f
226 2014-05-27T18:13:17 *** sl33k_
227 2014-05-27T18:15:12 *** sl33k_
228 2014-05-27T18:38:58 *** magu_cic
229 2014-05-27T18:39:55 *** magu_cic
230 2014-05-27T18:57:20 *** aboutGod
231 2014-05-27T19:02:26 *** aboutGod
232 2014-05-27T19:46:27 *** RogerHaase
233 2014-05-27T21:21:14 *** dave_largo
234 2014-05-27T22:13:29 *** sl33k_
235 2014-05-27T22:18:15 *** sl33k_
236 2014-05-27T23:41:39 *** magu_cic_
237 2014-05-27T23:42:51 *** magu_cic
238
