After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 623974 - [PATCH] Year field in track editor dialog should not start at 0
[PATCH] Year field in track editor dialog should not start at 0
Status: RESOLVED FIXED
Product: banshee
Classification: Other
Component: Track Editor
git master
Other Linux
: Normal enhancement
: 1.x
Assigned To: Banshee Maintainers
Banshee Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-07-09 18:01 UTC by Chow Loong Jin
Modified: 2012-09-13 18:36 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
More intelligent year field (2.75 KB, patch)
2010-11-11 15:16 UTC, Samuel Gyger (IRC: thinkabout)
reviewed Details | Review
Cleaning the Coding style (1.43 KB, patch)
2010-11-12 06:11 UTC, Samuel Gyger (IRC: thinkabout)
needs-work Details | Review
Changes made as proposed. (2.78 KB, patch)
2010-11-17 21:45 UTC, Samuel Gyger (IRC: thinkabout)
needs-work Details | Review
Fixed some Hacking Problems (2.76 KB, patch)
2011-02-02 21:03 UTC, Samuel Gyger (IRC: thinkabout)
needs-work Details | Review
Hides the input of the textfield when year is 0. (1.44 KB, patch)
2012-08-28 10:13 UTC, Samuel Gyger (IRC: thinkabout)
committed Details | Review

Description Chow Loong Jin 2010-07-09 18:01:06 UTC
Originally reported by mlissner on https://bugs.launchpad.net/bugs/603471:

In the track editor dialog, if a year is unknown for a song, it defaults to 0, which is fine, but it would be better if it defaulted to hyphen, because then it would allow the spinners to start at a number near the year of the song.

For example, if it's at zero now, and I click the up spinner, it goes to the year 1BCE. This isn't so good. If it started with hyphen, and I clicked the up arrow and it went to 2000, that would be better. Subsequent down presses could go to 1999, 1998, etc., and up presses could simply work as usual.

It's very trivial as bugs go, but would be an improvement to the UI.
Comment 1 Mike 2010-07-09 18:27:25 UTC
Thanks for getting this filed here. Much appreciated.
Comment 2 Samuel Gyger (IRC: thinkabout) 2010-11-09 13:29:01 UTC
How do we differ between not set and the year 0?
Comment 3 Andrés G. Aragoneses (IRC: knocte) 2010-11-09 13:45:40 UTC
I think there's no difference. For all purposes, 0 means "not set" in ID3 fields.
Comment 4 Mike 2010-11-09 21:37:30 UTC
Don't know about the back end, but on the front end I was imagining hyphen to indicate null, and 0 to indicate the year 0. 

Currently this isn't possible (0 == Null), but there's no logical reason to set it to zero anyway. As far as I know, we don't have any music from then that we've digitally remastered...not yet anyway.
Comment 5 Andrés G. Aragoneses (IRC: knocte) 2010-11-09 21:44:52 UTC
(In reply to comment #4)
> Don't know about the back end, but on the front end I was imagining hyphen to
> indicate null, and 0 to indicate the year 0. 

Yep, something like that goes very in line with what is requested in bug 407406, although it may be difficult to hack without modifying Gtk+.

> Currently this isn't possible (0 == Null), but there's no logical reason to set
> it to zero anyway. As far as I know, we don't have any music from then that
> we've digitally remastered...not yet anyway.

Agreed.
Comment 6 Chow Loong Jin 2010-11-10 05:30:20 UTC
I think it should be possible to set the value to 0, and call set_visibility(false) and set_invisible_char('-') on the spin button to get the effect of it being "-".
Comment 7 Samuel Gyger (IRC: thinkabout) 2010-11-10 06:31:19 UTC
I tested some possibilities, but it's only possible with hacking the backend of the SpinButton by creating a new type or something like this.

The idea of Chow Loong Jin, could work. We could always set 0 to -, then it's not possible to set the year 0 and on the next change we start with 2000 or 1999.
Comment 8 Samuel Gyger (IRC: thinkabout) 2010-11-11 15:16:17 UTC
Created attachment 174257 [details] [review]
More intelligent year field

In Banshee.Gui.TrackEditor.SpinButtonEntry the onChanged Function is overridden to update the SpinButton every time someone types something into it, this violates the normal behaviour or? Is it really necessary. I removed this and attached a patch with the year field working as proposed.
Comment 9 Andrés G. Aragoneses (IRC: knocte) 2010-11-11 21:40:07 UTC
Comment on attachment 174257 [details] [review]
More intelligent year field


Great patch! Some small nitpicks though:

year_entry.Numeric = true;
+            year_entry.Output += delegate(object o, OutputArgs args) {
+                if(0 == year_entry.ValueAsInt) {

Please put a space between the "if" and the "(". You can read about these rules in the HACKING file.

+                    year_entry.Text = "-";
+                }
+                else if (100 > year_entry.ValueAsInt) {

This seems not explicitly said in HACKING, but we tend to put the else in the same line as the closing brace.

+                    year_entry.Value = 2000;
+                }
+                else {

Same here.

+                    year_entry.Text = year_entry.Value.ToString();
+                }
+                args.RetVal = 1;
+            };
Comment 10 Samuel Gyger (IRC: thinkabout) 2010-11-12 06:11:53 UTC
Created attachment 174304 [details] [review]
Cleaning the Coding style

Cleaning up after the last patch.
I read the HACKING File but forgot this one space. :D
Comment 11 Gabriel Burt 2010-11-17 20:25:21 UTC
Review of attachment 174304 [details] [review]:

Please collapse your patches for this bug into one patch, and add a good commit msg (http://live.gnome.org/Git/CommitMessages)
Comment 12 Samuel Gyger (IRC: thinkabout) 2010-11-17 21:45:09 UTC
Created attachment 174722 [details] [review]
Changes made as proposed.

Instead of - it shows a empty field, like the other boxes now.
Comment 13 Samuel Gyger (IRC: thinkabout) 2010-11-29 06:14:40 UTC
Any progress on this one? Patch merge?
Comment 14 Alexander Kojevnikov 2010-12-07 07:30:14 UTC
Review of attachment 174722 [details] [review]:

Sorry for delay in reviewing the patch. A few nitpicks:

::: src/Core/Banshee.ThickClient/Banshee.Gui.TrackEditor/BasicTrackDetailsPage.cs
@@ +186,3 @@
             SpinButtonEntry year_entry = new SpinButtonEntry (0, 3000, 1);
             year_entry.Numeric = true;
+            year_entry.Output += delegate(object o, OutputArgs args) {

Space after `delegate` please, as per HACKING. Or use `+= (o, args) => {`

@@ +189,3 @@
+                if (0 == year_entry.ValueAsInt) {
+                    year_entry.Text = "";
+                } else if (100 > year_entry.ValueAsInt) {

Why do you have `100 >` here and not just `1 ==`?

@@ +192,3 @@
+                    year_entry.Value = 2000;
+                } else {
+                    year_entry.Text = year_entry.Value.ToString();

Space before `(`.

::: src/Core/Banshee.ThickClient/Banshee.Gui.TrackEditor/SpinButtonEntry.cs
@@ +58,2 @@
             get { return base.Value; }
         }

Is this removal really necessary? Seems to work fine with this chunk untouched. If it *is* necessary, did you test that other spin buttons work with it (bmp, range, etc)?
Comment 15 Samuel Gyger (IRC: thinkabout) 2010-12-07 07:48:53 UTC
Review of attachment 174722 [details] [review]:

Sorry, don't have a notebook at the moment, I'm in zambia (africa) and my notebook is broken, not so easy to get a new one. So can't change the patch at the moment.

::: src/Core/Banshee.ThickClient/Banshee.Gui.TrackEditor/BasicTrackDetailsPage.cs
@@ +189,3 @@
+                if (0 == year_entry.ValueAsInt) {
+                    year_entry.Text = "";
+                } else if (100 > year_entry.ValueAsInt) {

1 == should also work, but I thought its more logical to have higher point for change. Otherwise, why is is possible to set to year 2 but not to year 1.

::: src/Core/Banshee.ThickClient/Banshee.Gui.TrackEditor/SpinButtonEntry.cs
@@ +57,3 @@
             }
             get { return base.Value; }
         }

Otherwise typing 1 to start the year 1999 would not work, it would immediatly change to year 2000.
Comment 16 Samuel Gyger (IRC: thinkabout) 2011-02-02 21:03:02 UTC
Created attachment 179928 [details] [review]
Fixed some Hacking Problems

I now got a second hand notebook and I'm online again :D.

1)I changed to == 1, works too, was just a design decision by me.

2) The function must be removed, otherwise one can't enter any year starting by 1 (e.g. 1999). I can't find any problem with the other SpinBoxes after removing this function.
Comment 17 Samuel Gyger (IRC: thinkabout) 2011-02-21 18:38:35 UTC
Any progress on this bug?
Comment 18 Matt Sturgeon 2011-02-27 06:22:55 UTC
Requesting review
Comment 19 Gabriel Burt 2011-03-22 14:56:06 UTC
Review of attachment 179928 [details] [review]:

I'm not too fond of this patch.  Or for that matter, of trying to fix this "bug".  The only "bug" I'd see here is maybe we should blank the spin entry if the year is "0", which we could do in the AddField call for year_entry.

::: src/Core/Banshee.ThickClient/Banshee.Gui.TrackEditor/SpinButtonEntry.cs
@@ -60,3 @@
-
-        // Make sure the value is updated every time the text is changed, not just when the focus leaves
-        // this SpinButton, since that may be too late

Like this comment says, we need this function because otherwise the value might not get changed until too late, eg if the user activates the Save button via its keyboard shortcut while having the spin button focused
Comment 20 Samuel Gyger (IRC: thinkabout) 2011-03-22 15:30:45 UTC
I haven't found any way to close it with loosing data.
Comment 21 Samuel Gyger (IRC: thinkabout) 2011-04-06 12:02:28 UTC
Review of attachment 179928 [details] [review]:

I think it's a function that is nice when you use it the second time, at first you will be suprised, and then it should be nice.

::: src/Core/Banshee.ThickClient/Banshee.Gui.TrackEditor/SpinButtonEntry.cs
@@ -60,3 @@
-
-        // Make sure the value is updated every time the text is changed, not just when the focus leaves
-        // this SpinButton, since that may be too late

In the current Head, this is already gone. So I think it's not a deal breaker.
Comment 22 Samuel Gyger (IRC: thinkabout) 2011-04-06 19:29:24 UTC
My mistake, of course if I rebase my changes will be on the head and therefore it's gone. :D And I found the commit related with this lines: 3fdaeb29

The problem appears also with me, try to get a way around.
Comment 23 Samuel Gyger (IRC: thinkabout) 2012-08-28 10:13:19 UTC
Created attachment 222617 [details] [review]
Hides the input of the textfield when year is 0.

The other problems of this bug I wasn't able to fix, but as Gabriel said, it's perhaps not all too important. :)

Hiding 0 makes the behaviour consistent with bgo#407406.
Comment 24 Bertrand Lorentz 2012-09-13 18:36:01 UTC
Comment on attachment 222617 [details] [review]
Hides the input of the textfield when year is 0.

Samuel, thanks for the patch, and for your patience!
Committed.

FYI, looks like your git config doesn't know your name, the author is labeled as "unknown-user <samuel@gyger.at>"
I fixed that up when committing.
Comment 25 Bertrand Lorentz 2012-09-13 18:36:17 UTC
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.