close
Comments you submit will be routed for moderation. If you have an account, please log in first.
Modify

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#145 closed bug (fixed)

Different values for BirthdayCheckTime

Reported by: opiopi Owned by: opiopi
Priority: normal Milestone: YAM 2.7
Component: configuration Version: nightly build
Severity: minor Keywords:
Cc: OS Platform:
Blocked By: Blocking:
Release Notes:

Description

I get here every time i start YAM and save the config
different values for hidden value for BirthdayCheckTime.

step by step to reproduce:

  1. start YAM.
  2. go to config.
  3. save config.
  4. quit run.
  5. comare the BirthdayCheckTime value with the original value.
  6. do the steps 1-5 again.

At point 5 i get every time another value.

I had a short look into the source and i think it's
because of a uninitialized ds_Days in the DateStamp
structure but i'm too busy this week to look at this
deeper.

Used version: YAM 2.7-dev [OS3/m68k] (11.03.2010)
on a A2000 OS3.9BB2

BTW: would be nice i we can have more 'Component:'
options here e.g. for this ticket config(uration).

Attachments (3)

BirthdayCheckTime.diff (2.4 KB) - added by opiopi 4 years ago.
BirthdayCheckTime2.diff (2.7 KB) - added by opiopi 4 years ago.
BirthdayCheckTime3.diff (2.3 KB) - added by opiopi 4 years ago.

Download all attachments as: .zip

Change History (13)

Changed 4 years ago by opiopi

comment:1 Changed 4 years ago by opiopi

  • Component changed from undefined to configuration

I had a look at the sources and do a fix for this ticket.
I'll add this patch as BirthdayCheckTime.diff.

Is it ok if i commit this patch?

comment:2 Changed 4 years ago by damato

  • Owner set to opiopi
  • Status changed from new to assigned

The patch seems to look quite good. However, a few comments from my side:

  1. I think you don't need to set nextDS.ds_Tick = C->BirthdayCheckTime.ds_Tick because we don't need the Tick information really as the highest resolution of BirthdayCheckTime should be minutes.
  2. Instead of nulling the BirthdayCheckTime.ds_Days right before DateStamp2String or String2DateStamp shouldn't that be better be performed in these two functions in case DSS_TIME is used with only the time shown? IMHO this would be a more proper fix for this issue.

comment:3 Changed 4 years ago by opiopi

  1. Why don't use the seconds? The time is saved in hh.mm.ss so

we can easy use the seconds too. Otherwise it makes no sense to
save the seconds if we don't use them. So here are two solutions:

  1. Use the seconds like it is with the attached patch.
  2. Don't save the seconds at all.

I really prefer solution 1.

  1. The ...ds_Days initialisation is now done inside the 2 functions.

See attached patch BirthdayCheckTime2.diff.

Changed 4 years ago by opiopi

comment:4 Changed 4 years ago by damato

  1. Sorry, now I remember correctly :) Indeed we had planned to rework the DateStamp2String() function to output the time without seconds. Of course this would require a new DSS_XXXX enumeration value to differentiate between wanted seconds and not wanted seconds. So in case of (1) we indeed considered seconds not relevant because the BirthdateCheckTime doesn't require seconds at all. We don't require that much precision and thus it is better to not use them at all. So please change your patch to not save the seconds and do not use ds_Ticks.
  2. Looks better now. But have you checked that your change doesn't interfere with DSS_TIME usage in YAM_WR.c line 1507 (source:/trunk/src/YAM_WR.c#L1507)

comment:5 Changed 4 years ago by opiopi

Ok i changed back and don't set the Ticks for nextDS.
I ask myself why you make sometimes things more complicated
than it's needed. It was working ok with the patch...

Yes of course i checked if the functions work like expected.
At least my tests don't show any negative effects.

I'll commit the patch very soon.

Not saving the seconds is not related to this bug and is really
another issue. Please go on and open a new ticket for that.
Thanks.

Changed 4 years ago by opiopi

comment:6 Changed 4 years ago by opiopi

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [4653]) * YAM_UT.c: Initialize the DateStamp->ds_Days in the functions

String2DateStamp() and DateStamp2String() if DSS_TIME is used.
This closes #145.

  • YAM_AB.c: Do the birthday compare only if here is really anthing to compare.
  • YAM_COs.c: minor cosmetic change.

comment:7 Changed 4 years ago by damato

Thanks for the fix.

And I am sorry if my comments sound annoying or pedantic to you. But please understand that for Thore and me not only the reasoning that it has to *work* is important. Of course your initial patch also worked, but we always want to ensure that we fix or implement things in a fashion that in future things are more reusable. We want to implement things in a nice and professional way and thus putting the ds_Days initialization into DateStamp2String() is IMHO the more nice and better solution to avoid that future uses run into the same problem. The same applies for the issue with not showing the seconds in BirthdayCheckTime. If you think about the case you have to acknowledge that seconds are not relevant for that use case. But when you show it to the user you normally have to implement it that it respects seconds as well. So the only sensible solution is to implement a new DSS_XXXX enum variable which allows to show the time without seconds and IMHO this would provide us additional functionality which we sooner or later will use anyway at some point. So my proposal for going that way instead of simply setting ds_Tick would help us in many ways ;)

comment:8 Changed 4 years ago by opiopi

I have no Problems with your comments. It's of course better to fix that issue inside the functions. What i mean is the seconds 'problem'. I don't understand exactly why it *must* be changed to not use the seconds. With saving and using the seconds it was working flawless and no more work was needed.

Another issue is the birthdaycheck use mui_request and it would be better to use here a nonblocking (async) requester. Is here anything in the TODO list or should i try to develop such requester for the birthday checking?

comment:9 Changed 4 years ago by damato

Well, you should ask yourself why should seconds be relevant for that case. And IMHO seconds are not needed and we can not even guarantee this high precision. So it would be more user friendly to just show "HH:MM" instead of using seconds as well which are not relevant at all. That's why Thore implemented it without taking care of ds_Ticks and that's why we thought it would be good to have another DSS_XXX value to just show the time without seconds which might become useful sooner or later anyway.

Regarding the birthdaycheck requester. Feel free to come up with something more advanced. But I would probably suggest to use our MUI classes system (mui directory) in case you want to create an own non-blocking requester. We are moving more and more to the new classes system and new MUI code should always use that instead of putting such stuff in the old-style YAM_XXX.c files.

comment:10 Changed 4 years ago by damato

  • Milestone set to YAM 2.7

Add Comment

Modify Ticket

Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.

This list contains all users that will be notified about changes made to this ticket.

These roles will be notified: Reporter, Owner, Subscriber

  • Frank Weber(Reporter, Owner, Participant)