ApplETS / ETSMobile-Android2

Portail étudiant mobile regroupant les principaux services accessibles aux étudiants de l'École de technologie supérieure.
http://clubapplets.ca
Apache License 2.0
12 stars 11 forks source link

Events feature #64

Closed ttauveron closed 7 years ago

ttauveron commented 7 years ago

Demo available here :

Demo available on youtube

zaclimon commented 7 years ago

Je ne suis pas familier avec le code encore du backend donc je ne peux pas réellement commenter. Par contre, je suis curieux à propos d'un certain choix que tu as fait:

Est-ce qu'il y avait une raison pour la librairie que tu as utilisé pour le bouton circulaire étant donné qu'elle est déprécié? J'avais comme première hypothèse l'animation du bouton en soi et comme deuxième le fait que le Design Support Library n'est pas utilisé dans l'app encore (D'où un FAB aurait peut-être été mieux approprié)

Sinon, tout semble joli. Good job! :+1:

ttauveron commented 7 years ago

Oui la librairie du bouton est dépréciée car elle n'est plus maintenue, j'ai fait quelques tests et pour le cas d'utilisation simple des events, tout fonctionne bien. Dans le pire des cas, le code est open source donc ça peut être fixé mais j'ai effectivement trouvé l'animation jolie et l'intégration facile dans ÉTSMobile (je suis pas un grand fan de développement d'animations sur Android haha). La personne conseillait de prendre un fab mais je ne voulais pas me compliquer la vie avec ça, surtout que la librairie semble bien fonctionner, pas de issue majeur, pas mal d'étoiles etc.

zaclimon commented 7 years ago

Je suis pas mal certain qu'utiliser un FAB ne serait pas compliqué mais encore là, je crois que c'est plus quelque chose qui sera utilisé lors du changement de design éventuel. J'ai plus tendance à utiliser le moins de librairies possible lorsqu'il le faut mais tant qu'elle fait bien le boulot tout le monde est content! 😄

Sinon j'ai fait un build rapide et j'ai noté deux choses (surement relié à la même chose)

Dans l'un ou l'autre des cas, ça indique probablement que l'information n'est pas sauvegardé dans l'appareil. Est-ce que tu penses que ça serait bon de peut-être mettre l'information en cache dans l'appareil ou tu préfères mieux fetch l'information à chaque fois que l'utilisateur va dans le Fragment en soi?

Btw désolé si je sonne un peu critique dans mes remarques XD.

ttauveron commented 7 years ago

Faire du caching serait définitivement une belle amélioration aux événements!

Personnellement, j'ai tendance à utiliser le plus de librairies possibles pour aller plus vite, j'ai vu cette librairie qui pourrait être intéressante : https://github.com/lowlevel-studios/storo

Peut être créer un nouveau ticket pour ça !

ttauveron commented 7 years ago

Je viens de tester ce matin et il y a un petit problème : les évènements d'hier sont affichés. Je pense que le problème vient du serveur qui n'est pas à la bonne heure. Il va falloir changer ça et vérifier!

ttauveron commented 7 years ago

Bon il y a autre chose du côté API qui retourne certains événements déjà passés, je vais faire un check sur ETSMobile de la date avant affichage quand j'aurais le temps

ttauveron commented 7 years ago

Après investigation tout est normal finalement, c'est juste que les événements qui ne sont pas encore terminés s'affichent quand même

zaclimon commented 7 years ago

La librairie semble intéressante mais ça ne serait pas mieux de mettre les informations déjà cachés dans l'ORM? Aussi on pourrait utiliser la fonction de cache de Picasso pour sauvegarder les images. (Elle semble déjà supporter LRUCache d'ailleurs donc on attribue une photo avec une clé)

Pour les événements, afin d'éviter toute confusion concernant le déroulement, est-ce qu'un statut "en cours" ferait du sens?

ttauveron commented 7 years ago

Finalement, je suis pas convaincu par cette lib, j'ai utilisé le cache fourni par Robospice mais qui nécessite une connexion internet, c'est sensé accélérer beaucoup le rechargement des requêtes (cache de 10 minutes).

Picasso, si je me trompe pas, fait déjà un caching donc il ne devrait pas y avoir de problèmes à ce niveau.

L'ORM c'est bien pour la synchronisation avancée, mais, par rapport aux efforts pour développer une synchro versus le besoin de la fonctionnalité (les événements en temps réel), je mettrais plutôt en place quelque chose de moins overkill. Ça nécessite une connexion internet mais j'ai l'impression que c'est mieux que rouler la synchro en background.

Je pense que j'irais pas plus loin pour le moment avec cette fonctionnalité, en attendant de voir comment elle est reçue par les utilisateurs et, si besoin, elle pourrait être améliorée!

zaclimon commented 7 years ago

Effectivement j'ai remarqué une différence au niveau du chargement avec le commit concernant Robospice.

Pour ce qui est de Picasso, ça dépend de si un Picasso.Builder.downloader a été utilisé quand t'es allé chercher les images (Il cache juste initialement en RAM et pas sur disque)

http://stackoverflow.com/a/24959931

C'est intéressant que tu parles des besoins de synchro vu que je voyais ça différemment. (Les événements de la semaine par exemple vs les événements en temps réel.) J'ai proposé uniquement d'utiliser l'ORM à la place d'une autre solution uniquement parce qu'elle était déjà disponible.(Pourquoi réinventer la roue plusieurs fois? ;) )

Pour ce qui est de la fonctionnalité en soi, c'est toi qui voit. Si tu souhaites le pousser, vas-y!

ttauveron commented 7 years ago

Allons-y pour la fusion, si ça marche bien, il y aura de la place pour l'amélioration !