Re: Adding integration with more online dive logs GSoC 2015
On Wed, Mar 11, 2015 at 4:32 PM, Dirk Hohndel d...@hohndel.org wrote: On Wed, Mar 11, 2015 at 07:31:47PM +0200, Yosef Hamza wrote: a) Left it as implemented first for now -safer- b) Done but when passing the gaschange to the helper C function the compiler raised a warning, I guessed that's not acceptable, So I did this hack. QString char *. Is that okay? qPrintable( qstring_variable ) ___ subsurface mailing list subsurface@subsurface-divelog.org http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
Re: Adding integration with more online dive logs GSoC 2015
On Tue, Mar 10, 2015 at 11:08:14PM +0200, Yosef Hamza wrote:
Check this and let me know what you think.
This is for the second one, I will leave the first one for now and check
the new changes made there by Tomaz.
The code flow has changed quite a bit for that one, so yes, read what's
there before continuing.
Did you resend the one that deals with negative time? The code was right,
just the commit message didn't seem so great...
diff --git a/qt-ui/profile/profilewidget2.cpp
b/qt-ui/profile/profilewidget2.cpp
index cfcd25d..17a2eb9 100644
--- a/qt-ui/profile/profilewidget2.cpp
+++ b/qt-ui/profile/profilewidget2.cpp
@@ -1383,6 +1383,20 @@ void ProfileWidget2::changeGas()
struct gasmix gasmix;
int seconds = timeAxis-valueAt(scenePos);
+ if (seconds == 0) {
+ struct event **events = current_dc-events;
+ struct event *temp;
+ while (*events) {
+ temp = (*events)-next;
+ if (event_is_gaschange(*events)
(*events)-time.seconds == 0) {
+ remove_event(*events);
+ mark_divelist_changed(true);
+ replot();
+ }
+ *events = temp;
+ }
+ }
+
Let me nit-pick a little :-)
a) the events are ordered by time, so the moment you have one where
time.seconds 0 you can stop
b) even easier, we have a helper function you could use:
get_next_event(ev, gaschange);
so you call this with current_dc-events
if you get an event back AND its time stamp is 0, remove that event
try again, again with current_dc-events as pointer
repeat until you don't find a gas change
c) I wouldn't call replot() in the middle; while it's unlikely that there
would be two gas changes at t=0, still one replot() would be sufficient,
right? So I'd have a little flag (bool eventRemoved) and then call
mark_divelist_changed() and replot() if eventRemoved is true.
Makes sense?
I know that this might feel frustrating. You keep sending patches and I
keep criticizing the code and not taking them. This really is intended to
help you and to make sure you understand the code and the code we have in
the end makes sense. Sure, I could just fix this myself (that would be a
lot less work for me) - but it's unlikely that you'd learn something if I
did that.
/D
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
Re: Adding integration with more online dive logs GSoC 2015
a) Left it as implemented first for now -safer- b) Done but when passing the gaschange to the helper C function the compiler raised a warning, I guessed that's not acceptable, So I did this hack. QString char *. Is that okay? c) Done §Yousef On Wed, Mar 11, 2015 at 12:20 AM, Dirk Hohndel d...@hohndel.org wrote: On Wed, Mar 11, 2015 at 12:13:17AM +0200, Yosef Hamza wrote: On Tue, Mar 10, 2015 at 11:39 PM, Dirk Hohndel d...@hohndel.org wrote: On Tue, Mar 10, 2015 at 11:08:14PM +0200, Yosef Hamza wrote: a) the events are ordered by time, so the moment you have one where time.seconds 0 you can stop Yea I thought about that, But Gehad told me before that there was an issue that can make events be out of order. There is? That would be bad. Gehad, can you comment on that? b) even easier, we have a helper function you could use: get_next_event(ev, gaschange); so you call this with current_dc-events if you get an event back AND its time stamp is 0, remove that event try again, again with current_dc-events as pointer repeat until you don't find a gas change That's a better solution indeed I didn't know about, I need to surf the helper functions more.. We have a lot of them. And not well structured, not consistently named, not well documented. There's almost a GSoC project hiding here in code cleanup... but the problem is that this would have to be done by a rather experienced developer whom I really trust... hard challenge for GSoC... c) I wouldn't call replot() in the middle; while it's unlikely that there would be two gas changes at t=0, still one replot() would be sufficient, right? So I'd have a little flag (bool eventRemoved) and then call mark_divelist_changed() and replot() if eventRemoved is true. Makes sense? Yes. Excellent. I know that this might feel frustrating. You keep sending patches and I keep criticizing the code and not taking them. This really is intended to help you and to make sure you understand the code and the code we have in the end makes sense. Sure, I could just fix this myself (that would be a lot less work for me) - but it's unlikely that you'd learn something if I did that. No not at all, It give me valuable checkmarks when submitting patches :) :-) /D 0001-Removing-gas-change-events-0-00-when-new-one-is-adde.patch Description: Binary data ___ subsurface mailing list subsurface@subsurface-divelog.org http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
Re: Adding integration with more online dive logs GSoC 2015
On Tue, Mar 10, 2015 at 5:02 PM, Dirk Hohndel d...@hohndel.org wrote: On Tue, Mar 10, 2015 at 02:54:48PM +0200, Yosef Hamza wrote: Hey subsurface, My name is Yousef Hamza, and I'm 3rd year CESS student, I already have contributions to subsurface the desktop app and the iOS app. You have sent some ideas for the iOS app and you still need to actually implement what I asked you to do in the code review. So I think it's a bit misleading to claim that you already have contributions. I think there has been a misunderstanding. I'm speaking generally, here is my Github account: https://github.com/yousefhamza. You can find me among the contributors on both subsurface and iOS companion app. About the code review for the patches that are still in progress, I'm waiting your reply on both. §Yousef ___ subsurface mailing list subsurface@subsurface-divelog.org http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
Re: Adding integration with more online dive logs GSoC 2015
On Wed, Mar 11, 2015 at 12:13:17AM +0200, Yosef Hamza wrote: On Tue, Mar 10, 2015 at 11:39 PM, Dirk Hohndel d...@hohndel.org wrote: On Tue, Mar 10, 2015 at 11:08:14PM +0200, Yosef Hamza wrote: a) the events are ordered by time, so the moment you have one where time.seconds 0 you can stop Yea I thought about that, But Gehad told me before that there was an issue that can make events be out of order. There is? That would be bad. Gehad, can you comment on that? b) even easier, we have a helper function you could use: get_next_event(ev, gaschange); so you call this with current_dc-events if you get an event back AND its time stamp is 0, remove that event try again, again with current_dc-events as pointer repeat until you don't find a gas change That's a better solution indeed I didn't know about, I need to surf the helper functions more.. We have a lot of them. And not well structured, not consistently named, not well documented. There's almost a GSoC project hiding here in code cleanup... but the problem is that this would have to be done by a rather experienced developer whom I really trust... hard challenge for GSoC... c) I wouldn't call replot() in the middle; while it's unlikely that there would be two gas changes at t=0, still one replot() would be sufficient, right? So I'd have a little flag (bool eventRemoved) and then call mark_divelist_changed() and replot() if eventRemoved is true. Makes sense? Yes. Excellent. I know that this might feel frustrating. You keep sending patches and I keep criticizing the code and not taking them. This really is intended to help you and to make sure you understand the code and the code we have in the end makes sense. Sure, I could just fix this myself (that would be a lot less work for me) - but it's unlikely that you'd learn something if I did that. No not at all, It give me valuable checkmarks when submitting patches :) :-) /D ___ subsurface mailing list subsurface@subsurface-divelog.org http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
