Quite a lot

[pablobc-mx]

Major features are that it allows searching for channels and playlists, as well as opening them in a different buffer.

[grastello]

Wow that's quite a lot, probably much of a lot for a single PR as the codebase does not seem the same anymore. Can you detail exactly what kind of new features this PR adds? I think it would be a better choice to add each thing (searching for channels, searching playlists, etc.) separately; it's easier to test stuff incrementally as ytel will be easier to debug (and the code easier to navigate) at each step down the road.

On a side note adding new files (ytel-channel.el, ytel-playlist.el) requires changing the melpa recipe, as ytel is currently a single file repository. I would like to avoid this if possible (mainly just because it takes some time for the guys at melpa to merge PRs). Again, by implementing each new feature one after another instead of all togheter we can keep everything in a single file, for at least some number of features (until the thing becomes unmanageable and one has to split it up.)

[pablobc-mx]

Sure, I'll try to explain all the features I added as best as I can. As I said, the major features are that it allows you to search for channels or playlists, either by interactively calling ytel-search-type (to select from all valid options) or by calling ytel-show-videos, ytel-show-channels, or ytel-show-playlists. All it does is set the type field when querying the Invidious API. In order to display information correctly for channels and playlists, I made two new structures for each to store new fields, as well as new formatting functions to display these fields (such as subscriber count, number of videos) and insertion functions (ytel--insert-playlist and ytel--insert-channel). To insert results into the buffer according to their type, I wrote ytel-insert-entry which picks the appropriate function from an alist.

Besides that, I added the following:

• New faces for new fields as well as old ones.
• the ability to interactively sort the results of the current page, using the sorting criterion, by calling ytel-sort-videos.
• A function to copy the RSS feed of the author of the current entry.
• the option to insert icons to represent certain fields, such as view count, video count, etc. You can enable this option by setting the variable ytel-show-fancy-icons or by calling ytel-enable-fancy-icons. You can also customize the icons by modifying the ytel-icons variable.
• ytel-open-entry, a function to call a default action on an entry, depending on its type. To modify these actions you must set the appropiate ytel--default-*-action to a function or your choice. By default, these open channels and playlists using a mode defined in ytel-channel.el and ytel-playlist.el, respectively. For videos, it just echoes the title to the minibuffer. This function is bound to RET.
• The ytel-channel and ytel-playlist modes, which display all the videos from a channel or playlist. The reason they are separate modes are because I didn't want to have conflicting keybinds from ytel-mode. Because they're a different mode I put them in different files for better organization. For the most part, these just re-implement functions like ytel-search-next-page, ytel-draw-buffer, ytel-query, ytel-insert-video and ytel-sort to work correctly with channels and playlists.

Also, I fixed a bug that caused an error when trimming long strings with special characters in them.

I think that's it. I know there are a lot of changes you haven't tested so if you want I can add some of these features one by one, starting with ytel-search-type. We can also omit the ytel-playlist and ytel-channel modes in the meantime but I find them very useful. I've been using my branch of ytel for some time and I haven't really had any issues with the changes I implemented.

[grastello]

I fixed a bug that caused an error when trimming long strings with special characters in them.

Thank you, I think I was working locally on a better way to handle long strings of unicode characters but gave up halfway through. I believe it impossible to keep our columns nice under all combinations of weird symbols.

For what regars the cosmetic changes (icons, faces, etc.) I think it's better to save them for later as the actual features are more important (not to say that the act of voluntarily inserting unicode icons into a buffer scares me beyond imagination).

The default action thing and the RSS thing should probably not be part of ytel as "It's goal is to allow the user to collect the results of a YouTube search in an elfeed-like buffer and then manipulate them with Emacs Lisp"; if the user wants such a feature he can implement it for himself exactly as he wants it. I'd like to keep ytel a relatively simple thing whose point it to extended by the user.

Let's start by implementing a way to search for playlist and channels, without worrying about anything else. I have some ideas myself about another possible way to do so that I'll sketch in the comment below.

[grastello]

Currently (not in your fork) the *ytel* buffer contains videos that are stored under the ytel-videos variable. We can change that to something like ytel-current-entries (a more general name) and allow videos, playlists and channel to be displayed togheter as follows.

We define three structures, one for every type of entry, as you have done. Instead of putting directly the strucures in ytel-current-entries we put a pair (type, structure) where type will be a symbol to identify the type of the entry (either 'video, 'playlist or 'channel) and structure the structure itself. Essentially we're doing a sum-type manually.

Now we can display all elements of ytel-current-entries (this will of course sort of fuck up all of our formatting; my naive solution would be that of simplifying it down) thus allowing the us to pass "all" as type to the api as well.

At this point we don't need different functions to perform a search, we can keep using s to search and use other keys to set the type of result we want (such as a for "all", c for "channel", p for "playlists", v for videos).

On the user side this complicates things a bit because now it's not a given what ytel-current-entries contains and switching on the type is required. One could even write a function (maybe a macro), say on-video, that takes a function f that works on ytel-videos and transforms it into a function that takes the entry at point, checks if it is a video, runs f is the answer is yes and does nothing if the answer is no.

As far as exploring playlists and channels goes I think we can get by without other modes by setting some variable and adding some conditions to > and <. But don't quote me on this last thing.

[pablobc-mx]

Alright, I understand. I also didn't think Unicode characters was a good idea, given the different character widths and all. Maybe I'll figure out a better way to do it in the future, but as of now it works for me.

I also thought about implementing different types of entries that way, as a (type . entry) pair, but the user side complications put me off. I wasn't really fully satisfied with my implementation either, as I had to write several wrapper functions full of conditionals to act on an entry depending on the type. One way I think might solve it is using an 'abstract' entry structure that contains the type slot (and maybe other information like id, author, and even construct a url to make things easier) and make the other three structures inherit from this one. You can do this using the :include option. It's an idea, but tell me what you think.

[grastello]

An entity structure from which the other inherit seems a more elegant idea than brutally consing a type and a structure togheter. I honestly did not knew about it. I think the best thing is to put only type in it.

On the user side the problem isn't really that big as one can easily add an if, and we can provide those function building function I was talking about, if we really wish.

[pablobc-mx]

I think you could benefit more by adding other fields besides type to the base structure. At least for those fields all entries have in common, like title, id, author, and authorId (in the case of channels, the title and author would be the same, as well as the id and authorId fields). One way I can think of is making a function that returns an RSS feed for the current entry. Without a common function like ytel-entry-author we would need to make a conditional to return what is pretty much the same data (the authorId field of entry).

[grastello]

Not sure about title and id since if you get the title/id of something you will most likely also want to know what the thing actually is (one probably does not want to put the id of a playlist where a the id of a video is expected). However for author and authorId I think you're right, it might be cool to store them in the base structure.

[grastello]

I just realized that maybe we should just use the predicates ytel-video-p, ytel-playlist-p and ytel-channel-p that cl-defstruct defines automatically for us instead of inheriting from a base struct and all that stuff. For accessing the author and authorId, that are common to all the strucures, more conveniently we can provide a function ytel-entity-author that works for any entity.

(defun ytel-entity-author (entity)
  (cond ((ytel-video-p entity)
     (ytel-video-author entity))
    ((ytel-playlist-p entity)
     (ytel-playlist-author entity))
    ((ytel-channel-p entity)
     (ytel-channel-author entity))
    (t (error "ops"))))

[spiderbit]

@pablobc-mx somehow I have tomatoes on my eyes and did not see your pull request and because it's hidden in a separate branch I did not see your changes.

I just made my own pull request, maybe you can rebase your changes on top of my (smaller) changes, I personally think that rewriting / changing this project to use tabulated-list because it comes automatically with sorting functionality some mouse functionality, and string trimming functionality build-in without the need to code it on a low level for this project. Also it has good standards like pressing g = refresh (revert-buffer), that is a uniform emacs standard in most table based modes of emacs.

What do you think?

[spiderbit]

One objection of @gRastello was that is now multiple files and that this changes stuff with melpa, techincally there is no need to split it up in multiple files, in my kodi-remote I use 8 modes / seperate buffers with 1500 lines of code all in 1 file.

I can see that it can be a bit more cleaner to separate it in multiple files but on the other hand you always have to think in which file what is and switch to it. So maybe merging it back to 1 file would be a possibility?

Maybe I am here a bit a extremist and I know that modularization is classically a good thing, but it ended for me in a python project in like 30 files in 20 sub-folders which was a maintaining nightmare, so maybe I am to extreme in putting to much in 1 file but from other packages even if they separate things in different files, each file is much bigger than this project.

The seq package is 500 loc, transmission.el 2500 loc json 760

And while magit has like 50 files magit-status.el is 3500 lines long. So I think with around 800 lines of code including 3 times the preamble is a bit to early attempt to abstract code. Abstracting code is dangerous if you do it wrong it has a huge cost to change the structure, that's why I am after my python nightmare very skeptical of OOP.

So in favor of keeping the code more maintainable I would suggest to put all the files back to 1 file if that is not somehow a big effort? I know that sounds counter intuitive because I think most educational institutions or books praise having many small files but in my experience that leads to less maintainability not more.