D14813: Adding XPlanet Viewer

[Pino Toscano]

pino added a comment.


  In D14813#311236 <https://phabricator.kde.org/D14813#311236>, @lancaster wrote:
  
  > I can still make more changes to the code if needed.  Many of the things you mention though that are left are not actually my doing due to the fact that it was already that way in the existing code before I started.  If they need to be changed in this code, they should be changed elsewhere as well.  I did these things the way I thought we were already doing them.
  
  
  I understand that, OTOH it makes no sense to carry on bad code with copy&paste just because "somebody else did it".
  
  > i18n string puzzles
  > 
  >   I'm not sure what you mean by this.  I think I did the i18n translations for the tooltips and other messages the way they were done elsewhere.  I got rid of the words on the buttons.  There might be some more needed in the xplanet options.  Is that what you mean?  I'm not sure what you mean by puzzles.
  
  @yurchor already replied on this.
  Also, your code still does lookup/comparison of translated strings, which is very fragile (and the wrong thing to do anyway).
  
  > hardcoded /tmp, totally unsafe temporary file name creation for the FIFO
  > 
  >   I got this method of creating the temp fifo file from Servermanager.cpp.  I thought I did it the same way.  If it is wrong here, it is probably wrong there.
  
  Yes, hardcoding any directory like this is definitely wrong.
  
  > wrong handling of non local URLs (which in fact that not supported)
  > 
  >   This code was already in the ImageViewer for saving the file.  I didn't change it much.  Would it be better to copy the code for saving from one of the other places files are saved in KStars such as fits viewer?  If so, which code do you consider good?
  
  I already said why it is not correct: you ask the user for a remote URL, but the actual code does not support non-local URLs (and in fact it will break in that case).
  If you want to handle only local files, then ask the users for local paths, and handle strings (and not URLs) for paths.
  
  > hardcoded size for buttons
  > 
  >   Jasem has set the minimum and maximum size for the buttons to either 32 or 22 for a very large number of buttons in Ekos.  Basically any time he put icons on the buttons.  Is this not the way we are doing it?
  
  Hardcoding sizes for UI elements like icons, fonts, and colors is definitely a wrong idea: other than the possibility for users to customize them according to their liking, it matters a lot for accessibility purposes. This is also why I mentioned the "do not play with fonts" earlier.

REPOSITORY
  R321 KStars

REVISION DETAIL
  https://phabricator.kde.org/D14813

To: lancaster, mutlaqja, pino
Cc: yurchor, pino, mutlaqja, kde-edu, narvaez, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20180818/2eb1132f/attachment.html>