Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Selector for theme variant (default, light or dark) #361

Merged
merged 8 commits into from
Apr 25, 2021

Conversation

alexislozano
Copy link
Contributor

Issue: #360

This adds a selector for the theme variant in the advanced pane of the settings.

Capture d’écran du 2021-04-23 22-05-45

I have used the same wording as in Gnome terminal, that is:

  • Default (Par défaut)
  • Light (Clair)
  • Dark (Foncé)

@maoschanz
Copy link
Owner

maoschanz commented Apr 23, 2021

ah génial, merci ! par contre on va discuter en français ce sera plus simple mdr

  • les combobox je suis pas fan, notamment en mobile c'est pas très simple à utiliser, et sur bureau je trouve qu'on en change trop facilement la valeur en scrollant. Je préférais une section avec un titre et un container avec un groupe de boutons radio : ça remplit la même fonction, donc autant ne pas ajouter du code pour supporter un truc qui fait la même chose.
  • la méthode qui ajoute des groupes de boutons radio fait un container sur toute la largeur de la fenêtre et qui est adaptatif, et ça c'est intéressant parce que ça permet de se lâcher sur la longueur des chaînes traduisibles : la probabilité que des termes courts et génériques comme "dark", "light" ou "default" soient utilisés ailleurs n'est pas négligeable, et selon le contexte le même mot ne se traduit pas forcément d'une seule manière. En utilisant des boutons radio on peut passer à un truc plus explicite comme "dark variant" ou "dark theme variant" par exemple
  • les méthodes qui ne sont pas appelées en dehors de la classe (update_theme_variant) devraient commencer par un underscore (certes c'est pas exactement la convention standard du python, et je suis pas encore 100% cohérent moi-même sur le sujet, mais c'est un bon début que le nouveau code respecte un peu ça)
  • il y a une variable "label" mais elle ne sert pas à l'affichage, donc je ne dirais pas que c'est un label. Ce serait mieux qu'elle s'appelle value ou un truc comme ça
  • le fichier window.py est tellement gros qu'il est "découpé" en sections, je suppose que c'est subjectif comme rangement mais je pense qu'update_theme_variant aurait sa place plutôt dans la section "WINDOW DECORATIONS AND LAYOUTS" plutôt que "GENERAL PURPOSE METHODS"
  • désolé je suis très maniaque sur les lignes vides : il en faudrait preferences.py ligne 167, et aussi juste avant le add_section_separator. Et dans le fichier xml il y en a 2 à la suite il faudrait pas...

et puis un dernier problème mais qui vient de moi, c'est que je suis sur le point (bon... ça fait 3 semaines que je me dis ça mais je vais y arriver) de sortir la version 0.8.0, et je ne voudrais pas rajouter des chaînes de caractères aux traductions juste avant la date de sortie : elles ne pourraient jamais être traduites à temps. Du coup je ne mergerai cette branche que minimum la semaine prochaine, désolé

@alexislozano
Copy link
Contributor Author

@maoschanz Pas de souci pour les points soulevés, je ferai les modifs. Pour les traductions, pour les langues autres que français et anglais, je peux récupérer celles de Gnome Terminal, qu'en dis-tu ?

@maoschanz
Copy link
Owner

non je ne sais pas comment ça se passerait pour le copyright, on a sans doute le droit vu que c'est gpl mais ça impliquerait que je doive créditer, pour 4 mots, des dizaines et des dizaines de gens qui ne savent même pas que le projet existe

@alexislozano
Copy link
Contributor Author

@maoschanz Okay pas de souci !

@alexislozano
Copy link
Contributor Author

@maoschanz J'ai fait les changements demandés, voilà ce que ça donne :

Capture d’écran du 2021-04-24 01-09-49

@maoschanz
Copy link
Owner

ok parfait merci, je mergerai ça dès que la version 0.8.0 sera sortie du coup

@maoschanz maoschanz merged commit 25173bf into maoschanz:master Apr 25, 2021
@maoschanz
Copy link
Owner

maoschanz commented Apr 25, 2021

ah zut j'avais oublié une remarque, c'est qu'il aurait fallu compléter la section "contributeurs" du "à propos"

je vais le faire moi même, je mets "Alexis Lozano" ?

@alexislozano
Copy link
Contributor Author

@maoschanz Oui :)

maoschanz added a commit that referenced this pull request Apr 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants