[WIP] enhancement: Added a command-line option to list all the available color themes.

[sumagnadas]

This PR adds the --list-themes command-line option to zulip-term which list all the available color themes.

Fixes #613

[neiljp]

@sumagnadas Thanks for working on this 👍 I've left some minor comments inline.

As per your comment in #613, you have the right idea. To avoid the first commit breaking tests, squashing these commits together would be good.

You could also add a test for this functionality, and as per #613 perhaps add an entry in the FAQ regarding themes.

[neiljp]

We haven't used tabs in the rest of this file; we could do with standardizing/simplifying, but for now could you use spaces for consistency?

I'm not sure we need leading/trailing blank lines.

You could also indicate how the theme is specified after listing the themes?

[sumagnadas]

By specifying, do you mean specifying how to change the theme to the other ones using --theme or -t?.

[neiljp]

Yes, via that and possibly also the config file option.

[sumagnadas]

Themes available:-
    zt_dark (default) 
    gruvbox (gruvbox_dark) 
    light (zt_light) 
    blue (zt_blue) 

I changed the output and it outputs the themes like this now. Does this look good or did the previous one look better (or if you haven't seen the previous one, you can see the following output)?
Previous Output:-

Themes available:-
    zt_dark
    gruvbox_dark
    zt_light
    zt_blue

[neiljp]

@sumagnadas Regarding the output, the original was good for a first version, as they are the actual theme names. The other version you listed seems like a combination of the theme names and theme aliases, but not quite (the first is inverted)? The theme aliases are intended to be deprecated at some point, so I think it would be good to show only the official names, as originally.

However, the suffixes you added made me think we could mark the default theme with an [application default] suffix. An extension could be to add a similar note if a configuration file is loaded which is specifying a theme, and if -t if specified with --list-themes, show which theme would be loaded based on user preferences.

[sumagnadas]

The first one is inverted because the alias of zt_dark is default which doesn't sound like a name but more like a tag for the the first one.

I will remove the aliases part as it is going to be deprecated at some point as you said. I will print a line at the end describing how to change the theme using -t or in the zuliprc for permanently changing the theme. Something like

You can change the theme using "-t <theme>" or "--theme <theme>", or under "[zterm]" section in the variable "theme" in the .zuliprc file

[sumagnadas]

I added suffixes which denote the default theme and the theme specified or preferred by the user. I also added a description which tells the end-user how to change the theme using the command-line option or using the zuliprc file.

[neiljp]

@sumagnadas This looks good, but with the extra features/scope which I suggested this has grown quite a bit and we have some duplication - a little like my response!

I'd suggest you restructure the changes something like this:

• Make a relatively small commit similar to your previous version, with just the simple listing of themes given --list-themes, adjusted to show the default theme and with the changes (eg. to spacing) I asked for, and optionally that extra output at the end - that commit will be sufficient to minimally fix the issue and you can add "Fixes #613" to the body of this commit text to automatically close the issue when this commit is merged
• Note there is another location where themes are listed - it would be useful to remove duplication and use (maybe only some of) the improved version in both cases, so a good follow-up commit would be to do that
• The current status of your branch has duplication between the new code and that below it, so the next step could be to adjust the list-themes approach from the new two commits to use the existing codepath which finalizes the selected theme

Here's an observation of your current sequence of commits - apologies if you're already aware of this: currently they show the order of your work, which is often more applicable committing directly to the main/master branch with each subsequent commit adjusting previous work. However, when merging a sequence of commits as they are from a branch it's easier to review and understand if the commits you write are each updated and restructured to match a logical clean flow. For example, you might go back and fix an older commit in your branch to make it work as intended or add code to it, rather than add more and more 'fix' commits on top to make the total work right - for example, this means not excluding code you add in one commit on the branch and then remove completely later. This can depend on the workflow in a project - if a branch is squashed before merging then this is not relevant, though arguably could still also help review; here we use merge-with-rebase so all the commits you write end up in the project git log after merging (though maintainers occasionally adjust them slightly before merging).

You may want to save your existing work as another local branch before you do eg. an interactive rebase, but I'll leave you to restructure it unless you'd like some more advice - I'm happy to help :) I think I see you on chat.zulip.org, so feel free to drop by #zulip-terminal or other relevant streams and ask any questions there too.

[sumagnadas]

* Note there is another location where themes are listed - it would be useful to remove duplication and use (maybe only some of) the improved version in both cases, so a good follow-up commit would be to do that

I didn't see anywhere else where themes are listed.

* The current status of your branch has duplication between the new code and that below it, so the next step could be to adjust the list-themes approach from the new two commits to use the existing codepath which finalizes the selected theme

Yes there is some duplication like the variables like available_themes, zterm. One way to remove the duplication can be to declare the variables just before the list_themes conditional branch. Another way could be to move the conditional branch into the try block just after those variables are declared. Which one sounds or will look better?

[sumagnadas]

What is left now to implement to this PR? I can't think of any other thing right now.

[neiljp]

@sumagnadas Thanks for your work on this - as discussed in #zulip-terminal on chat.zulip.org, I adjusted your earlier commits and merged those separately first, since that was sufficient for an initial solution and I recommend we work on the follow-up in a separate PR 👍