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 734430 - MtpSource's Loading Status is Off-By-One
MtpSource's Loading Status is Off-By-One
Status: RESOLVED FIXED
Product: banshee
Classification: Other
Component: Device - MTP
git master
Other Linux
: Normal normal
: ---
Assigned To: Banshee Maintainers
Banshee Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-08-07 14:12 UTC by Nicholas Little
Modified: 2014-08-11 19:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add Status Messages for More Loading Operations (4.57 KB, patch)
2014-08-07 15:03 UTC, Nicholas Little
committed Details | Review

Description Nicholas Little 2014-08-07 14:12:37 UTC
As shown by adding some trace messages to the load sequence:

[14 Debug 14:58:55.108] Loading GT-I9100 - 1578 of 1587
[14 Debug 14:58:55.111] Loading GT-I9100 - 1579 of 1587
[14 Debug 14:58:55.114] Loading GT-I9100 - 1580 of 1587
[14 Debug 14:58:55.116] Loading GT-I9100 - 1581 of 1587
[14 Debug 14:58:55.118] Loading GT-I9100 - 1582 of 1587
[14 Debug 14:58:55.120] Loading GT-I9100 - 1583 of 1587
[14 Debug 14:58:55.122] Loading GT-I9100 - 1584 of 1587
[14 Debug 14:58:55.124] Loading GT-I9100 - 1585 of 1587
[14 Debug 14:58:55.130] Loading GT-I9100 - 1586 of 1587
[14 Debug 14:58:55.133] MtpSource: Clearing Empty Albums
[14 Debug 14:58:55.134] MtpSource: Matching Tracks

This causes the final status message "...1586 of 1587" to remain displayed while banshee does other MTP and database operations, making it look like the last track takes an age to evaluate.
Comment 1 Nicholas Little 2014-08-07 15:03:02 UTC
Created attachment 282820 [details] [review]
Add Status Messages for More Loading Operations
Comment 2 Andrés G. Aragoneses (IRC: knocte) 2014-08-11 16:17:56 UTC
Comment on attachment 282820 [details] [review]
Add Status Messages for More Loading Operations

> From a2c8a0b198b4f8fbeefbf6d68d05221d838d51fb Mon Sep 17 00:00:00 2001
> From: Nicholas Little <arealityfarbetween@googlemail.com>
> Date: Thu, 7 Aug 2014 15:55:32 +0100
> Subject: [PATCH] mtp: Improve status messaging on load (bgo#734430)

1) First letter of first word in the first line of the commit message should always be uppercase.
2) You're not modifying the "Mtp" library, so please use "Dap.Mtp" instead.
3) Rather call it "logging" instead of "messaging.


> 
> The loading operation for MTP involves enumerating files, clearing
> empty albums, writing track information to the database and reading
> playlists. Banshee only produces a status update for the first step, in
> addition the current never reaches the total (due to an off-by-one
> issue) so it appears that the last track takes a long time to enumerate
> and load.
> 
> This patch adds status messages for those operations so the user
> doesn't experience a long delay with the same message before being
> able to use his device. In addition, we move the calls to
> Catalog.GetString outside of their respective loop bodies as a small
> performance optimisation.
> ---
>  .../Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs   | 24 +++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs b/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs
> index 3b28602..2d895e7 100644
> --- a/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs
> +++ b/src/Dap/Banshee.Dap.Mtp/Banshee.Dap.Mtp/MtpSource.cs
> @@ -152,19 +152,18 @@ namespace Banshee.Dap.Mtp
>  
>          protected override void LoadFromDevice ()
>          {
> +            // Translators: {0} is the file currently being loaded
> +            // and {1} is the total # of files that will be loaded.
> +            String format = Catalog.GetString ("Reading File - {0} of {1}");

We use "string" instead of "String" when using it as a type (we use "String" when calling its static methods.


>              track_map = new Dictionary<long, Track> ();
>              try {
>                  List<Track> files = null;
>                  lock (mtp_device) {
>                      files = mtp_device.GetAllTracks (delegate (ulong current, ulong total, IntPtr data) {
> -                        //user_event.Progress = (double)current / total;
> -                        // Translators: {0} is the name of the MTP audio device (eg Gabe's Zen Player), {1} is the
> -                        // track currently being loaded, and {2} is the total # of tracks that will be loaded.
> -                        SetStatus (String.Format (Catalog.GetString ("Loading {0} - {1} of {2}"), Name, current, total), false);
> +                        SetStatus (String.Format (format, current + 1, total), false);
>                          return 0;
>                      });
>                  }
> -
>                  /*if (user_event.IsCancelRequested) {
>                      return;
>                  }*/

Unneeded whitespace change above.


> @@ -178,7 +177,12 @@ namespace Banshee.Dap.Mtp
>                      }
>                  }
>  
> -                foreach (Track mtp_track in files) {
> +                // Translators: {0} is the track currently being loaded
> +                // and {1} is the total # of tracks that will be loaded.
> +                format = Catalog.GetString ("Loading Track - {0} of {1}");
> +                for (int current = 0, total = files.Count; current < total; ++current) {
> +                    SetStatus (String.Format (format, current + 1, total), false);
> +                    Track mtp_track = files [current];
>                      long track_id;
>                      if ((track_id = DatabaseTrackInfo.GetTrackIdForUri (MtpTrackInfo.GetPathFromMtpTrack (mtp_track), DbId )) > 0) {
>                          track_map[track_id] = mtp_track;
> @@ -194,10 +198,16 @@ namespace Banshee.Dap.Mtp
>                      @"INSERT INTO CorePlaylistEntries (PlaylistID, TrackID)
>                          SELECT ?, TrackID FROM CoreTracks WHERE PrimarySourceID = ? AND ExternalID = ?");
>  
> +                // Translators: {0} is the playlist currently being loaded
> +                // and {1} is the total # of playlists that will be loaded.
> +                format = Catalog.GetString ("Loading Playlist - {0} of {1}");
>                  lock (mtp_device) {
>                      var playlists = mtp_device.GetPlaylists ();
>                      if (playlists != null) {
> -                        foreach (MTP.Playlist playlist in playlists) {
> +                        for (int current = 0, total = playlists.Count; current < total; ++current) {
> +                            MTP.Playlist playlist = playlists [current];
> +                            SetStatus (String.Format (format, current + 1, total), false);
> +                            Track mtp_track = files [current];
>                              PlaylistSource pl_src = new PlaylistSource (playlist.Name, this);
>                              pl_src.Save ();
>                              // TODO a transaction would make sense here (when the threading issue is fixed)
> -- 
> 1.8.5.5

Even though I don't like this kind of "mixed" patches (which change 2 things at once; I mean, you could have done one just to fix the off-by-one error, and another one for the rest), I've fixed the above nitpicks and pushed it to master. Thanks!
Comment 3 Andrés G. Aragoneses (IRC: knocte) 2014-08-11 16:18:43 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 and patch.
Comment 4 Andrés G. Aragoneses (IRC: knocte) 2014-08-11 18:02:01 UTC
BTW, I wonder if it's good to actually remove the device name from here (I know it was repetitive for the log, but it wasn't for the UI status message).

Can you cook a patch to bring up the device name for all messages, and use a non-translated (not using Catalog.GetString) message for the log which doesn't have the device name please?
Comment 5 Nicholas Little 2014-08-11 18:58:55 UTC
(In reply to comment #4)
> BTW, I wonder if it's good to actually remove the device name from here (I know
> it was repetitive for the log, but it wasn't for the UI status message).

Remembering that the user has to select the source name in the list to see the status messages, it felt a little repetitive in the user interface, for example in the DAP preferences screen the name is presented no less than 3 times, once in the source list, the large name next to the device icon and in the status bar.

It felt more like the name was present just to answer the question of what Banshee is loading, which the new patch addresses. 

If we drop the "file/track/playlist" from the message string then we'd have "Loading Name - x of y" shown for three different values of y, which I didn't think was ideal and is the reason I swapped it for file, track and playlist. If we kept the extra information then we'd have "Loading file/track/playlist from Name - x of y" which probably isn't too bad?

> Can you cook a patch to bring up the device name for all messages, and use a
> non-translated (not using Catalog.GetString) message for the log which doesn't
> have the device name please?

There was no logging there initially but I'll put some in and produce a patch, but I think just the discrete phases like Loading Files, Loading Tracks and Loading Playlists would be best to avoid filling up the log?