Created attachment 321596 [details] [review]
Search popover: limit options to max 5 years

The list displayed too many options for older dates.

To fix this I've modified the number of days for the loop that
generated those options. Besides this I've also removed two
conditions that are made redundant due to this change.

Review of attachment 321596 [details] [review]:

Thanks for the patch! It's almost good. See small fixes inline:

::: src/nautilus-search-popover.c
@@ +26,3 @@
 #include <libnautilus-private/nautilus-global-preferences.h>
 
+#define MAX_SEARCH_YEARS 5

Just a nitpick, we can search for more than 5 years. Just that the filter does not :)

How about the name for the constant be SEARCH_FILTER_MAX_YEARS?

@@ +381,3 @@
   maximum_dt = g_date_time_new_from_unix_local (0);
   now = g_date_time_new_now_local ();
+  max_days = (MAX_SEARCH_YEARS + 1) * 365;

Why +1?? We want effectively 5 years.

Created attachment 321842 [details] [review]
Search popover: limit options to max 5 years(update)

You're right about the constant name, so I've renamed it.

About the +1, The reason I used it it's because after the
first year, the variable days is equal to 371 so there is
a +6 because of the step. But if I change the step, there
wouldn't be displayed the same entries. I know it's not
the perfect solution, so if you consider something else
would be better I'll change it :)

(In reply to Alexandru Pandelea from comment #3)
> Created attachment 321842 [details] [review] [review]
> Search popover: limit options to max 5 years(update)
> 
> You're right about the constant name, so I've renamed it.
> 
> About the +1, The reason I used it it's because after the
> first year, the variable days is equal to 371 so there is
> a +6 because of the step. But if I change the step, there
> wouldn't be displayed the same entries. I know it's not
> the perfect solution, so if you consider something else
> would be better I'll change it :)

Sorry took long time to review. Now I'm more free.

So if the problem is the days var has an offset, remove it.

Maybe something like:
      else if (days < 365)
        {
          /* months */
          normalized = days / 30;
          /* Make sure we don't have an offset */
          if (normalized == 1)
            days = 30;
          step = 84;
        }

And also the step of the months is wrong (not your fault), you can change it to 90 if you want :)

Created attachment 322164 [details] [review]
Search popover: limit options to max 5 years(update 2)

Removing the offset in the else corresponding to years
solved the problem with the +1.

I've also changed the step to 90.

I hope that now it's ok, but if there's something that has
to be changed please tell me :)

Review of attachment 322164 [details] [review]:

Ok we are getting there! Few things:
The commit message contains a (update 2) thing, which shouldn't be part of it.
Also you report in the commit message things from a previous patch that are not longer needed. You need to do a single patch that fixes the issue, not a serie of patches.
Don't use the "I", instead use impersonal voice like: "Also fix the step value for every section".
Read more about commit guidelines here:
https://wiki.gnome.org/Newcomers/CodeContributionWorkflow#Commit_guidelines

Also a inline comment:

::: src/nautilus-search-popover.c
@@ +421,3 @@
           normalized = days / 365;
+          if (normalized == 1)
+          	days = 365;

This problem happens with every section, so I would add this to every one of them.
Also add a comment to explain why this is necesary. You can add it before the "while" statement

Created attachment 322233 [details] [review]
Search popover: limit options to max 5 years

The list displayed too many options for older dates.

In order to limit this, the max_days variable was decreased to
the number of days corresponding to 5 years

The value of the step for months was updated to the proper value, 90.

The days variable contained an unnecessary offset. To prevent it,
for the first occurrence of each timeslice (month, week, year) there
was added an if clause to remove the offset

Due to limiting the max number of years displayed, there was removed
(a now redundant) else from the function get_text_for_date_range.

Review of attachment 322233 [details] [review]:

Cool! That looks good now, thanks for the patch!

Attachment 322233 [details] pushed as 2cc130d - Search popover: limit options to max 5 years