Problems with the Listen Module

[soapdog]

There are many issues with the code inside the behavior_Listen script. I'm getting lost inside as I work to fix the journal handling routines. I'm creating this issue to document the problems encountered because keeping them all in my mind as I work the code is proving too hard.

After I fix the journal handling routine, I'll take a harder look at this issue to decide if I recommend fixing this behavior or actually rewriting from scratch.

[soapdog]

Too many script-local variables.

Severity: High

There are 26 script-local variables and 2 global variables in that script. No code should have that many script local variables. It makes it impossible to figure out where they are mutated and how their life-cycle works. You can't easily tell which handlers are using them and in which order changes are happening.

It is very easy to arrive in invalid states in which those variables don't hold the values the developer is expecting them to hold. For example, right now there are many routines that assume that a variable contains an item_id but that variable is actually empty. The code never checks for it and it causes cascading failures.

Also, there are script-local variables that are not declared on the top of the file, making it even harder to find that they exist.

---

Sample exposing the problem:

global sConfigA, sCardName
local sCollectionA,sCollectionChoice,  sSelectedItemA, sSelectedURL, sDownloadedAudioA
local sCurrentPlayListA,sTargetPlayList,sListType,sPlaylistQueryA,sShareURL,sOnLIne
local sNowPlayingTitle, sNowPlayingArtist,sNowPlayingDetails,sStartTimer,sEndTimer
local sCached, sBrowseHeader, sTimeStamp, sRemoteURL
local sSelectionsA,sLastTarget,sSelectNum, pDataA
local sFastforward

[soapdog]

Wrong usage of prefixes in variable names

Severity: High

This is the major causing of friction in my current work. The variables are simply named with the wrong prefixes. As explained in the scripting style guide from 4th world, we use a modified version of the hungarian notation described in that document. Our prefixes should be:

scope	prefix	sample
temporary	t	tTemporaryVariable
script-local	s	sCurrentPlayingAudio
global	g	gConfigA
handler parameter	p	pDataA

Besides those prefixes, we also use a suffix A to denote array variables.

Now, the source code for that module is consistently using the wrong prefixes. It is declaring global variables sometimes with s and other times with g. It has script-local variables declared with p, and handler parameters declared with s.

This makes it impossible to reason about the scope of the variables. You not only don't know the scope of a variable, you're misled by the wrong naming. Such as many routines making use of pDataA which should be a handler parameter holding an array but it is actually a script-local array.

---

sample exposing the problem:

on playList_FetchData tColl,tPlay,pFilterListQueryA,pBrowseHeader
   local tPlaylistClass, tRecordTypeCount, tDbaseColumns
   local  queryData, pJSON, tRecords, tSearchA, tFileID, tTocURL, tBookToc
   ...

In that sample there are many handler parameters declared with t when it should be p, temporary variables that should be declared using t have missed the prefix or are even using the wrong one such as p.

There are many many handlers with code with the wrong names in it such as this one:

command startPlayer pURL, pTitle, sCardName, sSelectNum, sTimeStamp
   local tRect, pAudioRect, localURL,  pItemID,tLineNum, tLineValue

Which causes script-local variables to be shadowed. From inside the scope of the command, they're declared as handler arguments, not script-locals with the same name. This causes buggy code because the variables being accessed are not the correct ones. Also, local declarations without prefix, or wrong prefix.

[soapdog]

Wrong naming of handlers

Severity: medium

The handler names are not consistent to best practices and in some cases are simply wrong doing the opposite of what their name says. A good example of this is the command called getPlaying which actually sets the value of the script-local playing variable instead of getting it. It should have been named setPlayingStatus or something similar.

[soapdog]

Too much reliance on side-effects

Severity: high

Too much of the code relies on mutating variables whose scope is larger than the running handler. It makes it hard to debug the code and understand the code because it is never clear when and where those variables are written and how their content dictates the flow of the app.

---

Example showing the problem:

on  playlist_Instantiate sTargetPlayList    
   Local tKeys, tTitles, pData

   # we have data from local or the server: 
   # turn off loader...

   showBusyIndicator false

   # using: model_SivaSiva, which has functions to get json info we need:

   switch sPlaylistQueryA["playlistClass"] 
   ...

Where was sPlaylistQueryA mutated? Which function fills it up? Same thing with most s-prefixed variables such as sCurrentPlayListA. We can't easily figure out the interactions and life-cycle of these 26 script local variables, many of which are multi-level arrays.

[soapdog]

Lack of error checking

Severity: high

As established before, the code is heavily based on the state of script-local variables and relies on side-effects to make everything work. Unfortunately there is almost no error checking code verifying the state of those variables. They're simply assumed to contain the correct value, and running the code through a debugger shows that they often don't contain the expected values.

A good example of this is in the fetchSingleMetadata command:

command fetchSingleMetadata pItemID
   local tSearchA,tTargetRecordA
   put pItemID  into tSearchA["item_id"]
   put  fetchMediaItems(tSearchA) into tTargetRecordA # returns only 1 record
   put tTargetRecordA[1] into sSelectedItemA
   metadata_SetRecord sSelectedItemA
   get getSongLyrics(pItemID)    
   put getMediaMeta() into it
end fetchSingleMetadata

When the user selects one of the downloaded audio files, this command is called with an empty parameter pItemID. This causes the tSearchA array — which is used to generate the SQL where clause for searching — to have a key called item_id with an empty value. This causes the SQL query to fetch the metadata to be created without a where-clause and makes the return value tTargetRecordA to actually contain all the items on the database.

The code that fills sSelectedItemA will always end up with the very first database record in it. Regardless of which audio the user is listening to because no one checked to see if pItemID contained a valid value.

There are many other cases like this in this source code. Running a debugger shows many of the script-local variables containing only half of the values they need.

This manifests in the current journal fix work in many ways. For example, the wrongly named pDataA script-local variable is assumed to have an item_id field. It does have this field, but it is empty. Causing the journal to be written without an item_id, making it impossible for journalResume to work because the database don't contain a reference to which audio item the user was listening when they saved the entry.

[soapdog]

A journal fix that should take a couple hours max is turning into a multiple days heavy work because the code is in such a bad state that it becomes impossible to figure out where things are going wrong and how.

[soapdog]

The same problems outlined above are also present in behavior_ListenSelectPlay.livecodescript. This makes the severity of the issues even stronger because there are side-effects working across different behavior using variables that are misnamed such as having globals called sConfigA and sCardName.

It should have been clear to all developers involved in the constructions of these modules that we shouldn't be using globals and should be restricting script-local variables as much as possible. The safest way to develop with LiveCode is using just temporary variables and handler arguments. Now, I have to do scavenge hunts in the source code because the side-effects that dictate the lifecycle of the module might be happening in another behaviour.

[soapdog]

Mixing handler arguments and non-temporary variables

Severity: High

The code was really strange because it declared script-local and global variables on top, and yet it passed the same names as handler parameters in many handlers, such as:

command startPlayer pURL, pTitle, sCardName, sSelectNum, sTimeStamp

It is not clear but on top of that file sCardName is declared as a global (should have been named gCardName), sSelectNum is declared as a script-local, and tLineValue should be a name for a temporary variable inside that handler. Unfortunately, all those variable names are causing the original variables to be shadowed.

LiveCode is filling those values from the handler invocation arguments, from inside the handler they will have the values passed by whichever handler invoked that command instead of the values set on the global and script-local variables of the same name.

In essence, what is actually happening is the same as having:

command startPlayer pURL, pTitle, pCardName, pSelectNum, pTimeStamp

All new variables. The other behavior who call these commands are passing those values. They are not from the wider-scope. Again this is a basic mistake that makes programming really really hard. I just spend hours refactoring that code to remove those strange argument declarations (you don't need to pass them as arguments if they're script-local or globals, just to realise that they're named completely wrong and being used by another file.

This is all happening because the LiveCode style-guide has not been used correctly, and the source is full of mistakes and bad practices.

[soapdog]

The issues extend to the libSivaSivaMedia.livecodescript as well.

In the screenshot above, we have the declaration of 33 script-local variables. There is only one variable with the correct name, all the other 32 variables are named with the wrong prefix.

This code is impossible to debug or reason about effectivelly.

[soapdog]

Whoever coded these modules named most variables badly, and many of the handlers as well.

So far that accounts for 2300+ lines of code across multiple files.

[soapdog]

There are cascading errors due to the metadata routines being naive. They in many cases fetch the wrong metadata, as in the metadata from a different item since there is no error checking in the source code and in many cases sSelectNum and an item_id end up being mixed in the running code.

Code in libSivaSivaMedia is using LIKE ... SQL clauses to compare integers, which leads to fetching the wrong data, and so on.

[soapdog]

Most of the UX drives the whole interacting from a single mouseUp handler in behavior_ListenSelectPlay that is used to arbitrate where the user clicked on the UI and uses a collection of hacks and side-effects to try to maintain some very fragile state.

A click might be the user selecting a navigation menu, an audio file, a collection, or even a UI control. They're all being handled on a 100+ lines command. This a bad pattern and adds to the friction in maintaining and debugging this code.

[soapdog]

Still working on untangling the listen module.

There were lots of other problems found in the module in the last 20 days but that I haven't documented here. Mostly related to variables being misnamed, such as when a variable called pItemID ends up containing the value of the clickline and so on. Part of this problem happened when I started renaming variables and removing what I perceived as erroneous parameters, such as declaring script-local variables as handler arguments, which is wrong, but was in fact mostly poor naming that caused shadowing of said variables, but that were indeed passed to many of those handlers.

---

At the moment I'm double checking the updateTimer command from lib_MobileControls. At least in the version I have here the math is wrong, or we're showing a millisecond accurate timer. Instead of hours : minutes : seconds, the update timer is showing minutes : seconds : milliseconds.

Another strange happening here is that the timer indicator is going back and forth at the beginning of the audio, this looks like it is being caused by multiple concurrent calls to updateTimer, as in more than one part of the behavior_Listen script is starting a updateTimer loop and we end up with multiple loops running. They're also running very fast, there is no need to update the timer ever 20 milliseconds. I believe that maybe those multiple concurrent loops with no termination condition are what caused the huge memory leak I posted on the use list earlier last week.

Anyhow, working on it.

[soapdog]

Similar problems are present in lib_MobileControls as well. For example, consider this small snippet:

function getCurrentTime 
   return sCurrentTime
end getCurrentTime

function getHoursMinsTime
   return tHoursMinutes
end getHoursMinsTime

local tMilliseconds, tHours, tMinutes, tSeconds

The first function is useless. It returns the value of a script-local variable, if that function is only used by that script and as can be seen on the screenshot showing the results of a stack-wide search, it appears to be the case:

It is just adding indirection and friction to the code. The updateTimer code has the line:

put getCurrentTime() into sCurrentTime

That is the same as writing put sCurrentTime into sCurrentTime, it has no effect whatsoever. The other function bellow is returning what appears to be a temporary variable that is never set, but that is not the case because that variable is declared with the wrong prefix. On line 52 of that script there are a bunch of variables all declared with wrong temporary prefixes or argument prefixes, when they should all be declared with an s for script-local. This is the kind of problem that makes me lose a ton of time as I track things down because they're not what they seem to be.

[Brahmanathaswami]

OK! found a tangle of code. Don"t be hard on me :-). Have you have time!

[bwmilby]

getCurrentTime is used in both the behavior and the library stack. The second one is only in the library. (I ran a project search using Atom since I have most scripts from binary stacks exported in the ScriptTracker tree as a double check - came up with same list)

[Brahmanathaswami]

Git Push? I did not push. But you, or I can, can push

We made a change to

behavior_ListenSelectPlay.livecodescript”

BR: we don’t need/want this dialog,

--dialog_CustomMsg “Go to My Downloads and take a timestamp for later in the Journal.”

2:18 PM brahmanathaswami

[Brahmanathaswami]

Git Push? I did not push. But you, or I can, push

We made a change to

behavior_Listen

command downloadSelection pSelectNum, pTarget setSelectNumber pSelectNum put the long id of pTarget into sLastTarget if sSelectNum <> empty then # we have a selection doAnswer "Download this audio file? Also, &cr& "Journal entry requires a download.", "Download,Cancel","downloadAffirmed" else put "Pick a Title to Download It." into fld "currentTitleLabel" end if end downloadSelection

[Brahmanathaswami]

Git Push? I did not push. But you, or I can, push

We made a change to

In grp “downloadControls” in behavior_Download; we hide the grp 'downloadSelection" as we have download it

on hidedDownloadControls local tLastStack put empty into fld "ProgressTitle" put empty into fld "progressField" hide scrollbar "progressbar" hide grp "downloadControls" hide grp "downloadSelection" end hidedDownloadControls`

[Brahmanathaswami]

The Desktop Version:

No files are going to the card "listen-my-audio" I call on property inspector on the fld "AudioList" it's text is empty.

The Mobile Version (because I have produced a standalone to my iPhone)

We ARE getting files that are downloaded, in card "listen-my-audio" field "AudioList"

[Brahmanathaswami]

THE Desktop version: we get a durattion

THE Mobile version: we see the duration show "00:00:00"

[Brahmanathaswami]

THE Desktop version, we get a journal entry, but only the first entry.

But not of the second entry, because (in my estimation) it playing from Journal. But it must have TWO entries (because the user may keeps, lets say three entry, one goes to a part that he really likes, the other one was the top entry, where he was only the last session.

There is no entry, only to the first one:

[Brahmanathaswami]

THE Mobile Version: Journal : we get a hard crash when we go the Journal. Perhaps it is coming in a console logging the timestamp. You would know in your Android version.

[soapdog]

I've picked previous comments and created issues for each of them. I'm working through each of them in turns but asked for clarification on #283