<table><tr><td style="">pino added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D14813">View Revision</a></tr></table><br /><div><div><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D14813#311236" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: line-through;">D14813#311236</a>, <a href="https://phabricator.kde.org/p/lancaster/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@lancaster</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>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.</p></div>
</blockquote>
<p>I understand that, OTOH it makes no sense to carry on bad code with copy&paste just because "somebody else did it".</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>i18n string puzzles</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">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.</pre></div></blockquote>
<p><a href="https://phabricator.kde.org/p/yurchor/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@yurchor</a> already replied on this.<br />
Also, your code still does lookup/comparison of translated strings, which is very fragile (and the wrong thing to do anyway).</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>hardcoded /tmp, totally unsafe temporary file name creation for the FIFO</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">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.</pre></div></blockquote>
<p>Yes, hardcoding any directory like this is definitely wrong.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>wrong handling of non local URLs (which in fact that not supported)</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">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?</pre></div></blockquote>
<p>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).<br />
If you want to handle only local files, then ask the users for local paths, and handle strings (and not URLs) for paths.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>hardcoded size for buttons</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">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?</pre></div></blockquote>
<p>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.</p></div></div><br /><div><strong>REPOSITORY</strong><div><div>R321 KStars</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D14813">https://phabricator.kde.org/D14813</a></div></div><br /><div><strong>To: </strong>lancaster, mutlaqja, pino<br /><strong>Cc: </strong>yurchor, pino, mutlaqja, kde-edu, narvaez, apol<br /></div>