schreibfaul1 / ESP32-MiniWebRadio

Internetradio with ESP32, I2S DAC and SPI TFT Display with Touchpad
https://www.youtube.com/watch?v=6QbPee2583o
299 stars 75 forks source link

Recent Changes in common.h #451

Closed cschwick closed 2 days ago

cschwick commented 1 month ago

Dear Wolle,

Recently there was a change in common.h (class dlnaList). This class had a function "hasClicked" in which the variable m_browseOnRelease was set (for example to '3' if the user clicked on a folder). Now I see that this functionality has been moved to "hasReleased". (94cd607 (5 days ago) was the last commit with the functionality in hasClicked)

Why am I asking?

I was just implementing a change for browsing the DLNA server so that the user could long-press on a folder and this would then on the fly create a playlist and play it. (I like listening to entire albums which I have organised in tracks in a folder on my server...) My change was just working (still some rough corners...) and I wanted to prepare it to propose it to you, however, when moving to the latest version in the master branch I saw the recent change mentioned above. With the change above the m_browseOnRelease is '0' in the longReleased callback function which I added to the dlnaList class. I was trying to check in this function if the long press/release of the user was done on a folder, and if so the creation of the On-the-fly playlist was triggered.

I might find a work-around, but was there a specific reason to move this functionality to the onRelease function? (...I guess so... but I do not see it since I am missing the overview....)

Thanks a lot, Chris

schreibfaul1 commented 1 month ago

Hi Chris, I've changed the scroll function a bit, it's enough to know the y-position at the start and end.

I like your idea of creating a playlist for the longPress event. I have changed the class, now a long press on the touchpad outputs the entry in the log. You can recognise whether it is a DLNA server name or a file, a folder or whether it was clicked outside the list.

cschwick commented 1 month ago

Great thanks! As soon as I have a bit of time I will look into it and adapt my code! I'll let you know!

cschwick commented 1 month ago

Hi Wolle, I am trying your recent changes concerning long-pressing objects in the dlna list:

It works great if I press objects in a list which are not the FIRST item in the list (e.g. not the first song in a folder, not the first folder in a list of folders). But If I long-press the first item in the list I get a crash: one of these: Guru Meditation Error: Core 1 panic'ed (LoadProhibited). Exception was unhandled. The crash comes before the message about the long press is printed out.

The reason for the crash seems to be that if you longpress on the first item in a subfolder, the m_itemListPos is calculated to be '0', but in the check for a file the vector m_srvContent.itemURL[m_itemListPos - 1] is indexed effectively with -1 which leads to a read of a "forbidden" memory region. I think the '-1' just needs to be removed (the printout statement in the next line does not have this -1 and the printout corresponds to the correct item being long-pressed). With the -1 removed all works fine for me. So I assume it was a small bug which somehow slipped in?

I now see if I get my modified code working with this. Looks promising to me :-)

cschwick commented 1 month ago

Dear Wolle,

The first version of the dlna-folder-player is in a forked github repo: https://github.com/cschwick/ESP32-MiniWebRadio-fork/tree/dlna-directory-play You have to use the branch: dlna-direactory-play If I long-click on a folder in my dlna server the radio plays the contents (and shows the titles & the number in the playlist)

However, there are flaws: I cannot change the bottom row of buttons to play the next/previous song. If I stop the playing the Player goes to the default playlist of the SD card. This needs further work so it sticks to the playlist created on the fly. But already like this it is usable (to some extent)...

Best wishes Chris

schreibfaul1 commented 1 month ago

Of course the array index must not be negative, [m_itemListPos - 1] is the problem, the -1 is rubbish. I'll have a look at your fork in the next few days.

schreibfaul1 commented 1 month ago

I have adopted your suggestions, and added two small changes: 1) the currently selected entry turns magenta if you press and hold it, 2) it waits until the DLNA client has the complete result and then the playlist is created.

This is not yet perfect, but it works. Many thanks for your help.

github-actions[bot] commented 2 weeks ago

This issue is stale because it has been open for 30 days with no activity.

github-actions[bot] commented 2 days ago

This issue was closed because it has been inactive for 14 days since being marked as stale.