Skip to content

Blog

Jump to bottom
Mike Scheutzow edited this page Nov 6, 2015 · 20 revisions

Brief thoughts and comments about my fork of Juk music player.

##2015-06-28 Working with the git version of Juk as of 2015-apr-12, I notice some things that need fixing:

  • I don't want my audio player sending information to the internet without my permission. A debugger shows socket activity related to song lyrics every time a new song starts playing, even when the lyrics pane is not visible.
  • while playing audio, the juk process leaks 1 to 2 MByte of memory every minute. valgrind shows me that the biggest leaks are coming from the phonon_gstreamer backend (v4.6.2).
  • juk takes ~25 seconds to create its system tray icon on a desktop directly using ALSA sound (i.e. pulse audio is disabled) but is quick if pulse audio is active.
  • juk doesn't detect any of my album covers that exist as separate .jpg files, even if I give them juk's preferred file name ("cover.jpg").
  • the cover manager dialog is not convenient to use; it seems to me that the entire dialog should not even exist.
  • 99% of my music collection is read-only, so it's wasteful that juk wants to access every single song file in my collection at startup.
  • who thought it was a good idea to prevent the GUI from drawing while building the initial database? It takes more than a minute on my system, making it appear that the application has locked up. Ever hear of a modal dialog to tell the user what's going on?

##2015-08-08 I've been working on various bug fixes, and have begun to push them to this project's public git repository.

  • improve the (visual) look of the Cover column in the track list.
  • fix: detect album covers which exist as standalone jpg image files.
  • fix: reliably save the app settings to the KDE config file.
  • workaround: eliminate long startup delay caused by SystemTray class.

##2015-08-12

  • change default LyricsWidget behavior to 'not shown'. It was annoying that it kept being enabled after I deleted the app config.
  • change the executable name to 'jukebox' so it can be installed at same time as original juk.

##2015-08-15

  • add some package dependency info to the "Build" page on wiki.
  • create a "Developers" page on wiki, and describe the application's class structure.

##2015-08-16 The commands on the Juk main menus are annoyingly general. The developers seem to assume I'll know exactly what they do. For example, File|Remove is particularly scary, and the GUI gives no help in figuring out what will happen if I click on it:

  • What is it going to remove? A playlist? A track? A tag?
  • Will it delete my disk file(s) too?
  • There's no '...', which implies I won't have a chance to cancel.

Months after I started using Juk, I discovered there was an actual manual available at Help|Juk Handbook. That manual has not been kept up-to-date, but is still good for understanding the app's capabilities. However, (1) nobody reads manuals, and (2) it does not completely answer the questions I ask above. All it has to say about this menu item is:

Remove the selected playlist.

That's a partial answer, but I still don't know if the command will delete my on-disk playlist, or the .mp3 files it points to. The only way to know is to try it. That's not very nice.

This particular item text should be "Remove Playlist...", and it would also help if we implemented QAction::setStatusTip() for at least the destructive commands. The hint could say something like "Delete playlist from app, ask about disk file".

This entry is about Juk source code tagged on 2015-02-21.

##2015-08-19 I've been looking at another data loss issue in Juk. It loses any unsaved Playlist changes when the user quits the app. Juk seems to assume the user will remember to click File|Save. Lame.

I see no code to remember that a Playlist has been modified, so at this point Juk isn't able to even show a dialog box as a reminder. If a dialog box is implemented, we need to handle the case that 1) the original .m3u is read-only, and 2) we shouldn't try to show a dialog box if the KDE session is ending (e.g. user is logging out.) For (2), I'm inclined to just silently write to the original .m3u file.

Oddly, Juk does save the modified playlist to its internal cache. But on the next app start, Juk gives precedence to the (old) .m3u file, and discards the cached data. An alternate solution would be to keep the cached content, if the cache file timestamp is more recent than the .m3u file timestamp.

This entry is about Juk source code tagged on 2015-02-21.

##2015-08-20 I'm still thinking about how to handle a modified playlist when the app quits. I like the traditional approach of a dirty flag + dialog prompt to save. The code is pretty clean, and the dirty flag could even be useful later for avoiding un-necessary writes to the cache files. But we still might experience data loss if the user abruptly logs out.

##2015-08-21 Commited my biggest patch yet today. On exit, prompts user to save if a Playlist object has changed.

##2015-08-27 All Playlist objects have a readOnly() method. This is "policy" i.e. a rule for a particular type of playlist. The meaning of "readOnly" is not documented, but from the code it seems intended to mean:

  • can not Cut or remove tracks from the playlist
  • can not Paste or Drag/Drop a track onto the playlist
  • can not delete the playlist from the app
  • can not save the playlist to a .m3u file
  • can not change the playlist name (the icon label)

Wow, that's very restrictive. This method returns true only for a single playlist (History). I'm thinking that if readOnly() is split up a little, e.g. canRename(), canModifyContent(), canDelete(), it'd help the menu-item enable code.

This entry is about Juk source code tagged on 2015-02-21.

##2015-08-28 How should my new UI methods map to the playlist types? These are intended to be policies for a specific class, not a current state.

Rename Modify Delete Reload Class Name
y y y y Playlist
n n n y CollectionList
n n n n DynamicPlaylist
n n n n HistoryPlaylist
y n y y FolderPlaylist
y n y y SearchPlaylist
n y n n UpcomingPlaylist

##2015-09-01

I don't think the current behavior of File|SaveAs makes sense. Its purpose is to write a .m3u file to disk with a user-specified name, which seems like a good feature.. However, the code also replaces the filename in the Playlist object, which is not so good. I've noticed these things about SaveAs:

  • when SaveAs is used on a playlist with a pre-existing m3u file, and the new m3u file is written to any Managed Folder, two playlists will show up the next time the Juk app is restarted
  • some Playlist class types remember the filename across app restart, and some do not (e.g. SearchPlaylist remembers until app exit, then forgets)
  • Save/SaveAs is allowed for any playlist type except History. Why the exception?
  • the Playlist object's filename is replaced whether or not the file write succeeded

The solution I like is to change "SaveAs" to "Export", and not overwrite the playlist filename. Note that File|Save still has to properly handle a couple special cases:

  • current filename is empty
  • current filename in the Playlist object is unusable (i.e. the file write failed.)

This entry is about Juk source code tagged on 2015-02-21.

##2015-09-02 I don't like it when a right-click Popup Menu is really long. I think that only the most frequent actions belong there, and the less-frequent stuff can live in the application's top Menu bar. In particular, the right-click menu for the PlaylistBox (the panel on the left side) has too much crap in it. Remove a bunch of actions, keeping only the 4 most useful. I'm leaving the 5th position available for a future "Properties" action.

##2015-09-04 I noticed the File|Import Playlist dialog box (formerly File|Open) won't show any .m3u files, and also has no "All Files (*.*)" filter. Leaving .m3u out is unexpected, because the underlying PlaylistCollection::open() method looks like it was written to handle .m3u playlist files just fine.

##2015-09-06

Adding *.m3u to the dialog box in MediaFiles::openDialog() was not difficult, but that exposed a serious bug in the code. The app would let you import an .m3u playlist this way, but would then forget it existed at next application startup. Tracking down the cause of that took many hours; it turns out the bug is in Cache::loadPlaylists().

Here's my commit message for the fix:

commit 869ac4150e8327c7190af0d730d59103d22f631d
Author: Mike Scheutzow <mjs973@gmail.com>
Date:   Sun Sep 6 12:05:26 2015 -0400

    fix: the app loses track of .m3u files external to 'managed folder' tree(s)
    
    In theory, the app caches the playlists that are shown in the left panel.
    But a serious logic error in Cache::loadPlaylists() caused the cached data
    for most playlists to be discarded.
    
    At app startup, checking if the playlist filename is present in the
    PlaylistCollection hash is wrong for two reasons:
    - the m3u fileName was just added to the hash by the operator>> immediately
      preceding the check. So if there's a fileName, the check returns true.
    - the folder scanner has not run yet, so the m3u filename can't possibly
      be in the hash list for that reason.
    
    So what the bad code *actually* does is to discard any cached playlist
    information that also stored an m3u filename. This is not the behavior
    we need.

This entry is about Juk source code tagged on 2015-02-21.

##2015-09-07

Juk is supposed to restore its previous volume level when the app starts up, but it has never worked for me. The in-application volume slider looks like it is set to the correct level, but when you actually click play it is way too loud. I tried changing to the Phonon vlc-backend, but that didn't fix this problem.

The explanation comes from Bug 321172, where it turns out that the problem is that Phonon ignores setVolume() if you're not in the Play State; Juk was trying to set it "too early". I'm using Phonon v4.6.2.

Commited a fix today.

[master 790e853] bug fix: at app startup, volume level not set correctly

This entry is about Juk source code tagged on 2015-02-21.

##2015-09-08 The mute button at the bottom of the in-application volume control behaves differently from the KMix volume control in the system tray. For the one in the system tray, the knob jumps to 0 if the mute button is clicked. Modify the behavior of our VolumePopupButton class to mimic this behavior.

Also, refactor VolumePopupButton, VolumeAction and TrackPositionAction to simplify code and add some comments.

##2015-09-11 I want to fix the track position slider. Hearing all the speaker-destroying clicks and pops as I drag the slider "puck" is really annoying. However, while I was implementing that, testing showed that something unexpected was happening with the periodic slider updates.

During normal playback, I was getting way more position updates than I expected (about 10 per second.) From reading the code, I expected one call every 800 milliseconds. I also noticed that the puck jumped around a bit near the beginning of each track if cross-fade was enabled. Here's the commit message for my fix.

commit 5a6b24895e6d6e97d43e6c9335de3668c99ccd63
Author: Mike Scheutzow <mjs973@gmail.com>
Date:   Sat Sep 12 07:53:33 2015 -0400

    playermanager: fix update of track time offset
    
    The PlayerManager provides a signal to periodicly inform listeners of
    the current time offset in the playing track. However, there were
    issues:
    
    - if cross-fade was enabled, PlayerManager sent conflicting time offset
      information *while* the cross-fade was in progress. The cause was both
      Phonon MediaObjects were invoking PlayerManager::slotTick(). This is
      a bug.
    
    - issued duplicate PlayerManager::tick() calls. While not technically
      invalid, it's inefficient and just causes unnecessary work for the
      listeners. The underlying cause is that a single Phonon MediaObject
      is calling tick() multiple times with an unchanged timestamp value.
      We workaround that by not propagating the duplicates out of
      PlayerManager.  Note: this problem was not cross-fade related.

This entry is about Juk source code tagged on 2015-02-21.

##2015-09-12 So back on 6/28 when I started documenting my work on this code, I complained that:

- I don't want my audio player sending information to the internet without my
permission. A debugger shows socket activity related to song lyrics every
time a new song starts playing, even when the lyrics pane is not visible.

However, I'm currently unable to replicate this bug. I suspect that it is related to disabling the LyricsWidget mid-request. Since I now default a brand new configuration to 'disabled', and never enable it, it doesn't seem to be a problem.

This entry is about Juk source code tagged on 2015-02-21.

##2015-09-13

I want jukebox to have a new behavior for a playlist which has a Read-Only .m3u file: the UI should prevent any modifications to that playlist. So no track adds or deletes, because these kind of changes can not be saved back to disk.

One way to implement this is to create a flag in the Playlist object which indicates if the playlist is modifiable or not. I'll call it isContentMutable().

At startup, or File|Import, the playlist file is checked, and the flag set appropriately. The enable/disable logic for menu items will have to check this flag, as will the Drag/Drop logic.

Less obviously, renames of the music files within the playlist should not be allowed. It also seems consistent to prevent such a Playlist from itself being deleted.

##2015-09-13

I've been looking at the internal Drag and Drop capability for tracks.

It's really annoying that during Drag/Drop, the app selects a playlist if the user hovers over its icon for a few seconds. Get rid of that behavior.

I also think it's real un-intuitive that user's can drop onto the background region of PlaylistBox. Get rid of that too.

##2015-09-17

Work on the top level Edit Menu, names and enable/disables. Remove "Cut" entirely. Change name of "Copy" to "Copy Tracks", and "Paste" to "Paste Tracks" to make it more apparent what they do if clicked.

##2015-09-19

For a while I've wanted the app to remember which playlist I had selected at exit, and restore it. Done now.

More fiddling with app initial volume level. The specified volume level just doesn't take, randomly. It's maddening. Try calling AudioOutput::setVolume() immediately AND when we enter the PlayingState. [Update: this fix didn't work either.]

Add last-modified timestamp to Playlist. We'll use it to choose between cache and .m3u file at app startup. Done.

##2015-09-20

Create a NormalPlaylist class to represent .m3u playlists. This'll let me make Playlist an abstract class, and move some methods higher in the hierarchy. Implement a Playlist::getType() method to cheaply identify the type of playlist.

##2015-09-22

More work on app initial volume level. Try calling AudioOutput::setVolume() when we enter Phonon BufferingState AND when we enter PlayingState. [Update: this fix didn't work 100%.]

##2015-09-26

Change Playlist class to allow duplicate tracks, except for CollectionList. Add better selection logic in PlaylistBox so we know which order user selected icons using ctrl-click. This lets us build a DynamicPlaylist in the same order as the user selection, which is much more natural. Remove PlaylistBox::m_doingMultiSelect variable and code since performance seems to be fine.

##2015-09-27

I discovered this morning that AudioOutput::setVolume() turns into a dbus message. Maybe the cause of this bug is a timing race further down the audio pipeline, so that when Phonon announces we're in PlayingState, it's not really true yet for the whole pipeline. I will try re-executing the setVolume() call 20 milliseconds after Phonon claims it enters PlayingState. We do this only the first time. [Update: 20 wasn't enough. Bumped to 50.]

##2015-09-29

Been digging more into the PlayerManager class. The 2 second delay to update GUI after stop is annoying me. Also, it's still emitting too many signals. What's the point of having both signalItemChanged() and signalPlay() issued so close together? Seems like we only need one of them.

##2015-10-01

More PlayerManager house cleaning. There's no reason to have the action enable/disable logic in the class itself, nor the window-title-setting logic. The class doesn't need to know about StatusLabel either, but we'll leave that for another day.

##2015-10-05

There's too much unnecessary method overloading going on in Playlist, PlaylistBox and PlaylistCollection. Fix the situation with raise(), setupPlaylist() and scanFolders().

##2015-10-10

Looking into weird behavior with StatusLabel update. Sometimes it seems to get 'stuck', and won't show the track summary info (# of tracks, total run-time.) Update: apparently, some developer thought this was desirable behavior after a track is dropped into the Playlist Queue. Stupidity. Restore the summary behavior when no track is actually being played.

##2015-10-11

Simplify the code by removing PlaylistInterface as a base class of Playlist. There's no good reason to do this.

##2015-10-16

I'm thinking user should not be permitted to modify a .m3u playlist that is read-only in the file system, because there's no way they can Save the changes back to disk. As another step towards that goal, don't allow drag/drop onto the icon of a read-only playlist.

##2015-10-18

A while back, I suppressed a dialog that popped up during 'File|Import' asking whether to add music tracks to the current playlist. It was annoying to be prompted when importing just a few songs. But it turns out, when importing a large number of songs, it's also annoying to have the unintended thing happen and get all this crap put in the current playlist. My compromise: ask only if 5 or more music files are imported.

##2015-10-21

Tighten up the rules for TreeViewItemPlaylist, which implements the leaf nodes under CollectionList in the PlaylistBox. Make them read-only, so that the edit search pattern can not be modified by the user, the tracks can not be modified, and the node can not be deleted.

##2015-10-22

Fix a nasty bug, where deleting one track from CollectionList would wipe out entire Album entry from the CollectionList Tree Mode.

##2015-10-29

In manual-width mode, I think it's more intuitive that all playlists use the same values. Change the code to share fixed column width for all playlists. Update: move the vector to SharedSettings, and persist it to the app config file.

##2015-10-31

In the Playlist class, the concept of columnOffset() is used to create 'extra' track table columns, beyond the typical ones ones listed on the View|Show Columns menu. However, these new columns are conceptually introduced on the left side of the table, which (when used) causes new column numbers to be assigned to all the standard columns.

This means there are x+offset and x-offset calculations sprinkled all over the code, and it's ugly as hell. To top it off, this feature is used in only one place in the whole app: the 'timestamp' column in HistoryPlaylist.

Just stop the craziness and eliminate the whole concept by conceptually adding any extra columns on the right. That means columnOffset() is always 0, and can simply be eliminated everywhere. Make it so.

Home

Clone this wiki locally