I guess there is no point of setting 2 alarms on exactly same time. So would it be okay that instead of warning user we don't allow user to create two alarms of exactly same scheduled time ?

Not allowing two alarms at the same time (and with the same settings for repeat) seems like a good thing. You could simply make the Done button (in the add alarm dialog) insensitive while the alarm to be added is the same as an existing one. One thing though - there should be feedback about why the button is insensitive (otherwise people might not understand). Maybe a simple label could be displayed - "You already have an alarm for this time" (or something like that).

(In reply to comment #2)
> Not allowing two alarms at the same time (and with the same settings for
> repeat) seems like a good thing. You could simply make the Done button (in the
> add alarm dialog) insensitive while the alarm to be added is the same as an
> existing one. One thing though - there should be feedback about why the button
> is insensitive (otherwise people might not understand). Maybe a simple label
> could be displayed - "You already have an alarm for this time" (or something
> like that).

Hi,
    I've tried to implement this but this will cost more time complexity than implementing something like whenever user clicks the Done button, we check for duplicate of the alarm and if it exists then simply pop-up warning box with simple label like as you mentioned "You already have an alarm for this time" and get user back to new alarm dialogue box.
    In this process too, suppose if user has set an alarm of 5:00pm repeating on Monday and Tuesday. Now if user tries to create a new alarm of 5:00pm repeating on Monday and Wednesday then should we allow user to create it ? If yes then both the alarm will ring at the same time on Monday at 5:00pm. In this case what should we do ?

(In reply to comment #2)
> Not allowing two alarms at the same time (and with the same settings for
> repeat) seems like a good thing. You could simply make the Done button (in the
> add alarm dialog) insensitive while the alarm to be added is the same as an
> existing one. One thing though - there should be feedback about why the button
> is insensitive (otherwise people might not understand). Maybe a simple label
> could be displayed - "You already have an alarm for this time" (or something
> like that).

Hi,
    I've tried to implement this but this will cost more time complexity than implementing something like whenever user clicks the Done button, we check for duplicate of the alarm and if it exists then simply pop-up warning box with simple label like as you mentioned "You already have an alarm for this time" and get user back to new alarm dialogue box.
    In this process too, suppose if user has set an alarm of 5:00pm repeating on Monday and Tuesday. Now if user tries to create a new alarm of 5:00pm repeating on Monday and Wednesday then should we allow user to create it ? If yes then both the alarm will ring at the same time on Monday at 5:00pm. In this case what should we do ?

Sorry for commenting the same thing again by mistake.

Created attachment 267801 [details] [review]
This prevents user from creating multiple alarms for the same time or editing and setting alarm time to any existing alarm time.

Thank you for your patch,

I also thought about this issue a little bit and I ended up with almost the same opinion as Allan.

The idea is to have a Done button insensitive by dialog creation is good. At least it correlates with the new dialog for clock creation, where we have an insensitive done button as well. I also do not see any implementation issues with this approach. One should just monitor hour- and minute-widgets for changes etc. and check if the configured alarm time is unique. if so the done button is turned clickable again and otherwise one could put a label within the revealer widget between active switcher and dialog buttons with a warning that a similar alarm configuration is already present. (and this check can be done by dialog creation as well)

(In reply to comment #6)
> Created an attachment (id=267801) [details] [review]
> This prevents user from creating multiple alarms for the same time or editing
> and setting alarm time to any existing alarm time.

Created attachment 268273 [details] [review]
This prevents user from creating multiple alarms for the same time or editing and setting alarm time to any existing alarm time by changing sensitivity of done button

I've to change scope of some widgets in SetupDialog to implement this. It is working fine now.

Review of attachment 268273 [details] [review]:

Thank you for your patch, it's almost good, just a small redesign required.

The main idea is to place the alarm setup related code in SetupDialog class, 
all alarm GUI related parts in and declarations of signal connections belong to alarmsetupdialog.ui.
This can be achieved for example by passing alarms member of MainPanel to SetupDialog constructor as a third item.
There are probably a much easier and better implementations ideas, What do you think Paolo?

::: src/alarm.vala
@@ +285,2 @@
     [GtkChild]
+    public Gtk.Switch active_switch;

278,280,284,286 I strongly believe that this SetupDialog member variable should stay private.

@@ +332,3 @@
+        Gtk.Box box = this.get_content_area ();
+        box.pack_start (warn_label, true, true, 0);
+        warn_label.set_no_show_all (true);

331-334:All widget related changes should be placed in
data/ui/alarmsetupdialog.ui

and we should experiment with the revealer widget which holds a warn_label. this will bring some sort of visualisation effect:)

@@ +659,3 @@
+        });
+
+            dialog.warn_label.hide ();

614-661 I think this part of code should move from MainPanel to SetupDialog, because it's actually a place where it belongs. This checks are needed for SetupDialog, and therefore it's reasonable to move this part there, so {h,m}_spinbuttons and day_buttons can remain private

Would it be okay if I declare signal connections in SetupDialog instead of declaring in alarmsetupdialog.ui file ?
Everything else has been implemented as you said but I'm having trouble in this thing as I have to pass Alarms list as an argument to signal handler which is tough to implement using glade as it only offers widgets of that ui file to pass as an argument.

Created attachment 268441 [details] [review]
This prevents user from creating multiple alarms for the same time or editing and setting alarm time to any existing alarm time by changing sensitivity of done button.

I've made some some modifications as suggested in review of previous patch. I've also used Gtk.Revealer as suggested.

Created attachment 268444 [details] [review]
This prevents user from creating multiple alarms for the same time or editing and setting alarm time to any existing alarm time by changing sensitivity of done button

I've made some some modifications as suggested in review of previous patch.
I've also used Gtk.Revealer as suggested. I've used deprecated signal "state-changed" in previous patch so I've obsoleted it and instead of that I've used "state-flags-changed" in this patch.

Review of attachment 268444 [details] [review]:

Nice work!
I hope that you do not think about me that I am really picky:)
Do you by the way have commit access?

::: data/ui/alarmsetupdialog.ui
@@ +188,3 @@
                   </packing>
                 </child>
+                <style>

Please move this <style> block back to 85-87 (to its original place) so it does not appear as a new change in the diff file.

@@ +286,3 @@
                       <placeholder/>
                     </child>
+                    <style>

same as stated above.

::: src/alarm.vala
@@ +290,3 @@
     private Gtk.Alignment am_pm_alignment;
     [GtkChild]
+	private Gtk.Revealer label_revealer;

Starting from here, please use spaces to indent the code instead of tabs. You was doing so before, but somehow not in this patch :)

@@ +298,3 @@
         Object (transient_for: parent, title: alarm != null ? _("Edit Alarm") : _("New Alarm"));
 
+		foreach (Item i in alarms) {

Is there any reason for holding a copy of alarms? cannot we just hold a reference to it, as we do not modify this list anyway, like alarms_list = alarms;

or just

Object (transient_for: parent, title: alarm != null ? _("Edit Alarm") : _("New Alarm"), alarms_list: alarms);

@@ +352,3 @@
+            label_revealer.set_reveal_child (true);
+        } else {
+

Do we really need this if statement?
will not just the line below do this task?
this.set_response_sensitive (1, true);

Created attachment 268496 [details] [review]
This prevents user from creating multiple alarms for the same time.

Hi,
    I've made changes that you suggested. For <style> block, I edited ui file in glade so the code was generated by glade so I didn't have idea about it. In this patch I've used unowned List<Item> so I can directly duplicate List instance passed as an argument to the constructor. Sorry for those indentation mistakes.
Thanks for suggestions. It helps me in doing things in a perfect manner.

Review of attachment 268496 [details] [review]:

Once improved, I think it's fine to be committed.

::: src/alarm.vala
@@ +125,3 @@
     }
 
+    public void update_alarm_time () {

Please consider this in combination with the comment below.
if we do not need this function in check_duplicate_alarm. please make it private again.

@@ +294,2 @@
     private Gtk.SizeGroup am_pm_sizegroup;
+    unowned List<Item> alarms_list;

please add 'private' here

@@ +356,3 @@
+
+    private bool check_duplicate_alarm (Item alarm) {
+            label_revealer.set_reveal_child (true);

I am not sure, why do we need to update alarm time here?

Created attachment 268672 [details] [review]
This prevents user from creating multiple alarms for the same time.

I've created new instance of alarm while checking its duplicate instance so update_alarm_time() method won't be called for this instance and its alarm_time property won't get set. If we want to keep this method private then we can call this in compare

but calling update_alarm_time in compare_item method will be called for every comparison and it will cost more so better if we make it public.

Review of attachment 268672 [details] [review]:

Ah, I see, sorry, you are right. you are free to push.


However if you still want to play with it, there is one thing we could try:
we could move check_duplicate_alarm to Item class

public bool check_duplicate (List<Item> alarms_list) {
  update_alarm_time ();
    foreach (Item i in alarms_list) {
      if (compare_with_item (i)) {
        return true;
      }
    }
  return false;
}

so we can keep update_alarm_time private.

and the line 348 turns into: if (alarm.check_duplicate (alarm_list)) {

what do you think about this idea?

Hi,
   Yes, I think it's good to move it to Item class and it is more suitable as it is a property of Item object. I'll make those changes and push it.

Created attachment 268706 [details] [review]
This prevents user from creating multiple alarms for the same time.

thanks Evgeny Bobkin for supporting and reviewing patches and pointing out my mistakes. I've learnt a lot.

Review of attachment 268706 [details] [review]:

Thank you for your contribution.

I just tested this patch and gnome-clocks crashes immediately when trying to edit an alarm.


I am not going to revert right away, but please investigate this and test more carefully

(In reply to comment #22)
> I just tested this patch and gnome-clocks crashes immediately when trying to
> edit an alarm.
> 
> 
> I am not going to revert right away, but please investigate this and test more
> carefully

hm, this is really strange, I cannot reproduce it at all, it works just fine here.

(In reply to comment #22)
> I just tested this patch and gnome-clocks crashes immediately when trying to
> edit an alarm.
> 
> 
> I am not going to revert right away, but please investigate this and test more
> carefully

This is working fine for me too. When I try to edit an alarm, it behaves in a normal way as it is supposed to.
Link for my testing: http://youtu.be/31QQI0fUayY

Sadly, it does not yet mean, that it is also has been written correctly :-P

(In reply to comment #24)
> (In reply to comment #22)
> > I just tested this patch and gnome-clocks crashes immediately when trying to
> > edit an alarm.
> > 
> > 
> > I am not going to revert right away, but please investigate this and test more
> > carefully
> 
> This is working fine for me too. When I try to edit an alarm, it behaves in a
> normal way as it is supposed to.
> Link for my testing: http://youtu.be/31QQI0fUayY

Created attachment 269046 [details] [review]
Modified Patch.

Created attachment 269052 [details] [review]
Latest relative to current master

Review of attachment 269052 [details] [review]:

I pushed the patch. For the future keep in mind to also change the commit message when working on a follow up patch

Created attachment 269423 [details] [review]
Do not show the warning label while editing an existing alarm - proposal

It turned to be not that trivial as I originally assumed (due to current design). Is there a better implementation way to the one I have proposed? Please review.

Created attachment 269427 [details] [review]
Do not show the warning label while editing an existing alarm - second proposal

Created attachment 269428 [details] [review]
Don't show warning label while editing an alarm.

This might be a better way.

Created attachment 269432 [details] [review]
Don't show warning label while editing an alarm.

This one is working as expected and not showing warning label for alarm_time of the alarm which we are editing. Done button is also sensitive on start-up.

Review of attachment 269428 [details] [review]:

pushed

Review of attachment 269423 [details] [review]:

I do not like it

Review of attachment 269428 [details] [review]:

oops. wrong one

Hi.. I was looking for a bug to solve for gsoc application and found this one. Currently the clock doesn't allow creation of multiple alarms for same time. But there can be times when a user is wanted to be reminded of multiple things at the same time i.e. there may me multiple alarms with different names required to ring at the same time. So wouldn't it be better if we allow creation of multiple alarms at the same time and  but ring only a single alarm. The user can stop/snooze it and see what alarms are on...?

(In reply to Abhishek Kumar from comment #36)
> Hi.. I was looking for a bug to solve for gsoc application and found this
> one. Currently the clock doesn't allow creation of multiple alarms for same
> time. But there can be times when a user is wanted to be reminded of
> multiple things at the same time i.e. there may me multiple alarms with
> different names required to ring at the same time. So wouldn't it be better
> if we allow creation of multiple alarms at the same time and  but ring only
> a single alarm. The user can stop/snooze it and see what alarms are on...?

"Ringing a single alarms and making user able to see what alarms are on" - I didn't get this part. If you're ringing only one alarm then may be you're talking about adding multiple names for the alarm which can be achieved by adding names to the same alarm's name and if you're allowing multiple alarms to ring or show pop-up each for one then in that case user will have to stop/snooze all the alarms and instead of that user will prefer only one alarm for that time.

In all cases only 1 ringing sound will be there.

It is inconvenient if a user is not allowed to create alarm just because there is another alarm with same time. The user may have created multiple alarms earlier and may not remember about each of them. Also the user may need this alarm for reminding himself about an entirely different purpose, which he will put as the name of the alarm. So, i am saying that creation of multiple alarms at the same time should  be allowed and is a vital feature.

Now once this is allowed, it can be handled either at the creation time of the alarms or the time at which the alarm(s) will ring. If handled at ringing time, we can see what multiple alarms are there and notify user that there are multiple alarms labels for the current ringing alarm . Specifically how this would be shown can be decided.

Also the above describe idea will work good in the case where there were two alarms initially set for different timings. The user snoozed one alarm and after snoozing the two alarms now get the same ringing time.

When creating a new alarm, we're checking "repetition days" and "active" property of the new alarm with the alarms having the same time and if at least one of the two properties is different then we're allowing to create a new alarm.

What you're saying can be achieved by comparing the name property too and if the name property is different and other 2 properties are same then may be we can merge their labels and don't create a new alarm or do something else. I've seen a few alarms apps and they don't allow multiple alarms for the same time and if they do then they don't manage very well to show all the alarms set for that time. 

I'm not sure if we want to modify this feature because user can simply edit an alarm and add label to its name. Still the app maintainer's word would be the final word on this.

(In reply to Saurabh_P from comment #40)
> When creating a new alarm, we're checking "repetition days" and "active"
> property of the new alarm with the alarms having the same time and if at
> least one of the two properties is different then we're allowing to create a
> new alarm.

So we don't handle the case when some repetitions of two distinct alarms fall together accidentally? That would speak for the other solution since it's more consistent.

(In reply to Saurabh_P from comment #40)
> When creating a new alarm, we're checking "repetition days" and "active"
> property of the new alarm with the alarms having the same time and if at
> least one of the two properties is different then we're allowing to create a
> new alarm.
> 
> What you're saying can be achieved by comparing the name property too and if
> the name property is different and other 2 properties are same then may be
> we can merge their labels and don't create a new alarm or do something else.
> I've seen a few alarms apps and they don't allow multiple alarms for the
> same time and if they do then they don't manage very well to show all the
> alarms set for that time. 
> 
> I'm not sure if we want to modify this feature because user can simply edit
> an alarm and add label to its name. Still the app maintainer's word would be
> the final word on this.

Any method of duplication check at the alarm-creation time is not a very good way as after repeated snoozing multiple alarms may get the same ringing time defeating the very purpose of not allowing multiple alarm creation by the user. The better way in my opinion is to notify user of the multiple labels that correspond to the currently active ringing sound. Like you said, many apps don't manage to show multiple alarm properly, but this can be improved here.

(In reply to Abhishek Kumar from comment #42)
> Any method of duplication check at the alarm-creation time is not a very
> good way as after repeated snoozing multiple alarms may get the same ringing
> time defeating the very purpose of not allowing multiple alarm creation by
> the user. The better way in my opinion is to notify user of the multiple
> labels that correspond to the currently active ringing sound. Like you said,
> many apps don't manage to show multiple alarm properly, but this can be
> improved here.

Usually it's good to look at what others do. E.g. google on android raises one of the alarms and shows the other as missed alarm so they basically just enforce
 * just one alarm can ring at a time
 * if an alarm can't ring it comes up as missed

that covers all cases (e.g. if the device is off) with very generic assumptions.

(In reply to Lasse Schuirmann from comment #43)
> (In reply to Abhishek Kumar from comment #42)
> > Any method of duplication check at the alarm-creation time is not a very
> > good way as after repeated snoozing multiple alarms may get the same ringing
> > time defeating the very purpose of not allowing multiple alarm creation by
> > the user. The better way in my opinion is to notify user of the multiple
> > labels that correspond to the currently active ringing sound. Like you said,
> > many apps don't manage to show multiple alarm properly, but this can be
> > improved here.
> 
> Usually it's good to look at what others do. E.g. google on android raises
> one of the alarms and shows the other as missed alarm so they basically just
> enforce
>  * just one alarm can ring at a time
>  * if an alarm can't ring it comes up as missed
> 
> that covers all cases (e.g. if the device is off) with very generic
> assumptions.

We can choose what is the better way of handling this.

a)showing missed: Is it a good way to deal with coinciding alarms on a pc? An alarm should be considered missed in cases like the pc was suspended or shutdown. 

b) Notifying the user of multiple labels of the currently ring: I think this is better than (a). It also handles the snooze cases. 

And in all cases there would be a single ringing sound and a single window to snooze/stop it.

Please let me know what do you feel about it.

And currently if there is a snoozed alarm , the user can create another alarm for the same time. This also the issue with the current fix.

This has been fixed by 3.38.