[GUI] Refresh ProjectInfoPage in attach wizard #2643

RichardHaselgrove · 2018-08-10T12:39:28Z

Use web_url when present
Parse all plan class names from
http://boinc.berkeley.edu/trac/wiki/AppPlan#Planclassnames
(except WSL)
Update AMD, NV GPU icons to modern style
Add icons for Linux/ARM and Intel_GPU
(icons downscaled from hi-res resource used for projects.php)
Re-sequence icons to group OS first, then resources
Add tooltips to all icons

Items 2-6 now match projects.php display. Implements #2579

1) Use web_url when present 2) Parse all plan class names from http://boinc.berkeley.edu/trac/wiki/AppPlan#Planclassnames (except WSL) 3) Update AMD, NV GPU icons to modern style 4) Add icons for Linux/ARM and Intel_GPU (icons downscaled from hi-res resource used for projects.php) 5) Re-sequence icons to group OS first, then resources 6) Add tooltips to all icons Items 2-6 now match projects.php display. Implements #2579

AenBleidd · 2018-08-10T15:13:31Z

It is not related directly to this ticket but anyway.
Does android icon look 'noisy' for me only (white dots on the 'chest')?

RichardHaselgrove · 2018-08-10T15:36:58Z

I think they're eyes (if above the shoulder), or a highlight. See https://boinc.berkeley.edu/images/android.png

AenBleidd · 2018-08-10T16:00:41Z

It is a highlight that looks not so fine on small icon with reduced number of colors.

RichardHaselgrove · 2018-08-10T16:21:39Z

I didn't update the Android icon this time - that's the original one. But I do know how to make an xpm format file now (fun!), so if we can find a better source image it could be done.

RichardHaselgrove · 2018-08-10T16:24:20Z

Something based on https://img13.androidappsapk.co/300/d/c/7/net.mannoun.androidoshistory.png, perhaps?

Ageless93 · 2018-08-10T16:32:19Z

More and free: http://pngimg.com/imgs/logos/android_logo/

RichardHaselgrove · 2018-08-10T18:04:08Z

I'm finding that gimp (required tool for xpm export) is completely over-the-top for the sort of image work we're doing here. The best I've come up with is

I'd prefer to leave it 'as is', otherwise this PR will languish for weeks like my previous attempts. And they want 7.14 out of the door...

JuhaSointusalo

These changes are related to each other in that they change the attach wizard so it makes sense to have them in the same pull request. However, the changes are independent of each other and should be put in separate commits.

JuhaSointusalo · 2018-08-10T19:29:04Z

clientgui/ProjectInfoPage.cpp

 #include "res/freebsdicon.xpm"
+#include "res/linuxarmicon2.xpm"


Why 2 in this and others?

To clearly differentiate my second generation icons from the previous set.

Ok, thanks for explaining.

JuhaSointusalo · 2018-08-10T19:30:16Z

clientgui/ProjectInfoPage.cpp

 #include "res/atiicon.xpm"
+#include "res/amdicon2.xpm"


Merge ATi and AMD here and in the rest of the code.

Code has to handle plan class names using either convention.

I had misunderstood your intentions. I thought you were going to have ATi and AMD icons both which would not have made much sense.

JuhaSointusalo · 2018-08-10T19:34:30Z

clientgui/ProjectInfoPage.cpp

 #include "res/atiicon.xpm"
+#include "res/amdicon2.xpm"
 #include "res/nvidiaicon.xpm"


If you are not going to use the old icon and want to have a new file with new name then remove the old one.

I wanted my PR to be non-destructive, at least until previous developers (Charlie and Rom) have had a chance to review.

JuhaSointusalo · 2018-08-10T19:36:19Z

clientgui/ProjectInfoPage.cpp

@@ -512,8 +559,9 @@ void CProjectInfoPage::OnProjectSelected( wxCommandEvent& WXUNUSED(event) ) {

        CProjectInfo* pProjectInfo = (CProjectInfo*)m_pProjectsCtrl->GetClientData(m_pProjectsCtrl->GetSelection());

-        wxString strURL = pProjectInfo->m_strURL;
-        EllipseStringIfNeeded(strURL, m_pProjectDetailsURLCtrl);
+//        wxString strURL = pProjectInfo->m_strURL;


Yes, it's probably safe to do that.

JuhaSointusalo · 2018-08-10T19:38:21Z

clientgui/ProjectInfoPage.cpp

                        pProjectInfo->m_bProjectSupportsCUDA = true;
 						if (!pDoc->state.host_info.coprocs.have_nvidia()) continue;
                    }

-                    if (strProjectPlatform.Find(_T("[ati")) != wxNOT_FOUND) {
+                    if (strProjectPlanClass.Find(_T("nvidia")) != wxNOT_FOUND) {
+                        pProjectInfo->m_bProjectSupportsIntelGPU = true;


Bug, though checking for nvidia makes sense to catch NVIDIA+OpenCL.

Good catch. I'll check for that and other copy'n'paste errors in the morning.

JuhaSointusalo · 2018-08-10T19:40:50Z

clientgui/res/amdicon2.xpm

@@ -0,0 +1,308 @@
+/* XPM */
+static char *amdicon2_xpm[] = {


These must be static const char* to avoid compiler warnings, even if it's against XPM spec.

The compiler objected to what GIMP had written. I tweaked it to match what the first generation icons already had.

Could you show the error message from compiler? The XPM data is made of string literals which are constant. Not having the data declared const generates tons of warnings. I think previously there was only one file not declaring the data const.

GIMP exports the line as
static char * D:\Sources_build\boinc_7_12_1\clientgui\res\androidicon2_xpm[] = {

That's clearly not usable, and VS2013 responds with

Error 3 error C2143: syntax error : missing ';' before ':' d:\sources_build\boinc_7-12-1\clientgui\res\androidicon2.xpm 2 1 boincmgr
Error 4 error C2059: syntax error : ':' d:\sources_build\boinc_7-12-1\clientgui\res\androidicon2.xpm 2 1 boincmgr
Error 5 error C2017: illegal escape sequence d:\sources_build\boinc_7-12-1\clientgui\res\androidicon2.xpm 2 1 boincmgr
Error 6 error C2146: syntax error : missing ';' before identifier 'boinc_7_12_1' d:\sources_build\boinc_7-12-1\clientgui\res\androidicon2.xpm 2 1 boincmgr
Error 7 error C4430: missing type specifier - int assumed. Note: C++ does not support default-int d:\sources_build\boinc_7-12-1\clientgui\res\androidicon2.xpm 2 1 boincmgr
Error 8 error C2146: syntax error : missing ';' before identifier 'clientgui' d:\sources_build\boinc_7-12-1\clientgui\res\androidicon2.xpm 2 1 boincmgr
Error 9 error C2146: syntax error : missing ';' before identifier 'res' d:\sources_build\boinc_7-12-1\clientgui\res\androidicon2.xpm 2 1 boincmgr
Error 10 error C2146: syntax error : missing ';' before identifier 'androidicon2_xpm' d:\sources_build\boinc_7-12-1\clientgui\res\androidicon2.xpm 2 1 boincmgr

I'll try again with const - I missed that specific point last time.

Yes, static const char *androidicon2_xpm[] = { is clean. I'll change it - I must have looked at the one file without const when fixing the first problem.

Yes, it was the original Android icon (which was added later, and doesn't have the licence lines either). I'll fix that while I'm here. Actually, no - I'll pull in that flat colour Android2 icon, to deal with Vitali's objection at the same time.

JuhaSointusalo · 2018-08-10T19:41:09Z

clientgui/res/amdicon2.xpm

+"o+p+q+r+s+t+u+v+w+x+y+z+A+B+C+D+E+F+G+",
+"H+I+J+K+L+M+N+O+P+Q+R+S+T+a+U+V+W+X+H+"};
+// This file is part of BOINC.
+// http://boinc.berkeley.edu


Then we'll have to change the old ones I copied from too.

I was aiming for having new files use https and old files could be mass changed in some other PR. I could have communicated that better...

JuhaSointusalo · 2018-08-10T19:41:20Z

clientgui/res/amdicon2.xpm

+// See the GNU Lesser General Public License for more details.
+//
+// You should have received a copy of the GNU Lesser General Public License
+// along with BOINC.  If not, see <http://www.gnu.org/licenses/>.


CharlieFenton

Please see my comments elsewhere under this PR.

CharlieFenton · 2018-08-11T09:45:01Z

While this is not entirely part of this PR, why is the code trying to translate URLs from English? I realize Richard was just expamnding on the first line below, which was added by @romw on June 15, 2011:
pProjectInfo->m_strURL = wxGetTranslation(wxString(m_apl->projects[i]->url.c_str(), wxConvUTF8)); pProjectInfo->m_strWebURL = wxGetTranslation(wxString(m_apl->projects[i]->web_url.c_str(), wxConvUTF8));

I haven't found any additional problems within the scope of this PR, but I see a few concerns with the logic in ProjectInfoPage.cpp.

[1] I think there is a shortcoming in the way the icons are displayed and in the way it warns that a select project may not have work for this type of computer. A project may support certain GPUs only on certain operating systems. Say a project supports only AMD on Windows and only NVIDIA on Macs. Then it should not display the AMD icon on a Mac, and should display the warning if selected on a Windows PC without an AMD GPU. The code now does not display the warning if the project has any support for the OS on which BOINC is currently running, even if there is no support for the GPU.

[2] I wonder how much sense it makes to show Linux and Mac icons on a Windows PC, etc. While this might be useful to the tiny percentage of BOINC users who have multiple computers with different operating systems, it might be confusing to most users.

[3] It is possible for a plan class to just have opencl without specifying the GPU brand; ideally OpenCL code should work on AMD, NVIDIA and Intel GPUs. In that case, should all 3 icons be displayed? Or should there be a separate icon for OpenCL support?

[4] While this PR adds checks for arm-unknown-linux-gnu and arm-unknown-linux-gnueabihf, the all_projects_list also has such entries as aarch64-unknown-linux-gnu and x86-android-linux-gnu. And all of these would trigger the display of the Linux icon and suppress the warning, even though applications for X86 won't run on ARM and vice-versa. But I realize there is a practical limit on how thorough this logic can be, especially as new combinations of hardware and OS continue to appear.

RichardHaselgrove · 2018-08-11T10:10:18Z

@CharlieFenton: I'm just starting work on the items Juha requested, but a quick initial response to your additional comments.

[1] I agree, but this shortcoming applies to the projects.php page on the BOINC website as well. "only AMD on Windows and only NVIDIA on Macs" should be visible in both places, but I think that's beyond the scope of this PR - one for the TODO list.

[2] Just a thought - how would this work if BOINC Manager is being used to control a remote PC, say a Windows tablet being used to control a Raspberry Pi? Again, food for thought, but outside the scope of this refresh.

[3] OpenCL runs on CPUs too, though I doubt any project would use it in preference to native apps. Developers have told me in the past that in most cases the applications do have to be separately compiled to suit the target GPU type, Again, this change would have to be applied to projects.php too.

[4] Again, I think that's out of scope here. Another for TODO, and an issue for BOINC v8.0? For the time being, I've checked, and all projects supporting Linux/ARM also support generic Linux on i686-pc and/or x86_64-pc, so no false information is being displayed.

Commented out the unused first generation icons Fixed nvidia detection bug Updated copyright and licensing for all files touched by this PR

RichardHaselgrove · 2018-08-11T11:23:41Z

@JuhaSointusalo I've pushed

Commented out the unused first generation icons
Fixed nvidia detection bug
Updated copyright and licensing for all files touched by this PR

I think that's as far as I want to go for a 'refresh': other queries, including those from @CharlieFenton, can perhaps await a full re-write?

RichardHaselgrove · 2018-08-11T11:43:18Z

Using https in the licence links opens a much bigger can of worms. While working on this PR, I've been referring to a copy of all_projects_list.xml (14-day autoupdate version) downloaded on 05 August 2018. It contains 34 project <web_url> entries - only 12 of which are https, and 22 still plain http. Although it's well documented that there are problems changing project master urls, that doesn't apply to either get_project_config or web display requests. I think we're guilty of a collective groupthink here, and we need to train ourselves out of it.

Of the 34 projects, just 8 can't be contacted by https from here. They are:

www.acousticsathome.ru/boinc/
sat.isa.ru/pdsat/
www.enigmaathome.net/
gerasim.boinc.ru/
srbase.my-firewall.org/sr5/
casathome.ihep.ac.cn/ (I can't reach that one by http either)
quakecatcher.net/sensor
radioactiveathome.org/boinc/

Downloading a fresh copy of all_projects_list.xml today, I find that all <web_url> elements have been removed again, so maybe David is working on that - though it's hard to know, because nothing has been committed yet. Correction - sorry: the copy I downloaded from the BOINC website wasn't the same as the 14-day autoupdate version. That still has the <web_url> entries, though I haven't counted the http entries yet.

Ageless93 · 2018-08-11T12:50:19Z

@CharlieFenton,
[2] How about running BOINC in a VM on another host OS? Which of the two OSes does it recognize?
But in that same frame of mind for the user who only has one OS, wouldn't it be overkill then to show the icon for the OS you're on? Because even though there could be other OSes supported by this project, you wouldn't care as you're not on any of those.

[3} Not all projects have OpenCL applications for all GPUs. Aside from that, one has to specifically install the drivers with OpenCL support for the device, else it won't be recognized as OpenCL capable, despite it being an industry standard. E.g. AMD GPU drivers do contain OpenCL, but this OpenCL doesn't allow BOINC to detect that the Intel CPU and GPU are OpenCL capable.

JuhaSointusalo

The code doesn't check for intel_gpu plan classes.

JuhaSointusalo · 2018-08-11T19:24:18Z

About this

I wanted my PR to be non-destructive, at least until previous developers (Charlie and Rom) have had a chance to review.

Now that you commented out the no longer used parts it's easier to understand what changes you wanted to do. But I think you could have just made the changes. The Files changed from the top of the page, as well as any other diff viewer, is designed for showing changes so there is no need to have the old code still around.

Another thing is that this PR is reviewed and eventually merged as is. If you have left some commented out old junk there then the junk gets merged too. Then there's the old XPMs that got replaced. It's confusing to have files that are really unused in source directory.

So I would very much like that you remove the unused code and XPMs.

RichardHaselgrove · 2018-08-11T20:29:32Z

The code doesn't check for intel_gpu plan classes.

I must have distracted myself when writing that bug you found first. I've put it back in, but on my development machine - which has both NVidia and Intel GPUs active - it doesn't show.

This is where my ignorance of C++ shows. Can somebody please talk me through the logic and purpose of the 'continue' statements in the detection area - around line 700? I'm worried that they may skip later tests if triggered (by no ATI in my case). The intel_gpu detection works if I comment them out.

I'll commit the code as it stands, but please treat this as a WIP until I can clarify what's going on.

Remove discarded files and code Include second-generation Android icon Use 'const' in icon files to eliminate Travis warnings Reinstate Intel_GPU detection, but this doesn't work on a machine with two active GPUs - see comment in #2643

RichardHaselgrove · 2018-08-12T17:44:16Z

After much careful scrutiny and abortive debugging (can't write to the message log from the Manager), I can't find what spooked me last night - unless it was LHC's [native_mt] plan class being picked up as ATI. Given that, I'll take off the WIP and invite reviewers to consider this as if ready to merge.

CharlieFenton · 2018-08-12T23:01:37Z

can't write to the message log from the Manager

You can add temporary printf() statements in the manager, and they will be written to the Manager's stdoutgui.txt file. I think that on Windows you will usually find that file at this location:
C:/Users/{username}/AppData/Roaming/BOINC/stdoutgui.txt.
That is where it is on my Windows 7 system.

CharlieFenton · 2018-08-12T23:02:44Z

@ChristianBeer: Can you please check why Travis CI builds are failing for all current PRs?

AenBleidd · 2018-08-12T23:12:26Z

@CharlieFenton, I'm not @ChristianBeer, but it looks like that there are some issues with Ubuntu image on TravisCI. I've restarted failed jobs

RichardHaselgrove · 2018-08-13T08:29:36Z

Well. 'abortive' in the sense of 'no bugs found', but I managed to get a GUI debugger working. Your way would have been quicker,

RichardHaselgrove requested review from davidpanderson, AenBleidd and CharlieFenton August 10, 2018 12:39

AenBleidd approved these changes Aug 10, 2018

View reviewed changes

JuhaSointusalo suggested changes Aug 10, 2018

View reviewed changes

CharlieFenton approved these changes Aug 11, 2018

View reviewed changes

Fixing ProjectInfoPage changes requested by @JuhaSointusalo

e7fc8ff

Commented out the unused first generation icons Fixed nvidia detection bug Updated copyright and licensing for all files touched by this PR

JuhaSointusalo suggested changes Aug 11, 2018

View reviewed changes

RichardHaselgrove changed the title ~~[GUI] Refresh ProjectInfoPage in attach wizard~~ [WIP] [GUI] Refresh ProjectInfoPage in attach wizard Aug 11, 2018

Further work on ProjInfoPage

aee2f9d

Remove discarded files and code Include second-generation Android icon Use 'const' in icon files to eliminate Travis warnings Reinstate Intel_GPU detection, but this doesn't work on a machine with two active GPUs - see comment in #2643

RichardHaselgrove changed the title ~~[WIP] [GUI] Refresh ProjectInfoPage in attach wizard~~ [GUI] Refresh ProjectInfoPage in attach wizard Aug 12, 2018

RichardHaselgrove added this to Review in Client Release 7.14 Aug 16, 2018

JuhaSointusalo approved these changes Aug 17, 2018

View reviewed changes

JuhaSointusalo merged commit 5e4111b into master Aug 17, 2018

RichardHaselgrove deleted the RDH_ProjInfoPage branch August 17, 2018 17:00

RichardHaselgrove mentioned this pull request Aug 18, 2018

[GUI] 'Add project' wizard: distinguish 'web' and 'attach' urls #2579

Closed

RichardHaselgrove moved this from Review to Done in Client Release 7.14 Aug 23, 2018

AenBleidd added this to the Client Release 7.14 milestone Oct 18, 2018

RichardHaselgrove mentioned this pull request May 7, 2020

Update URLs of projects to use HTTPS #3496

Closed

		#include "res/freebsdicon.xpm"
		#include "res/linuxarmicon2.xpm"

[GUI] Refresh ProjectInfoPage in attach wizard #2643

[GUI] Refresh ProjectInfoPage in attach wizard #2643

Conversation

RichardHaselgrove commented Aug 10, 2018

AenBleidd commented Aug 10, 2018 • edited Loading

RichardHaselgrove commented Aug 10, 2018

AenBleidd commented Aug 10, 2018

RichardHaselgrove commented Aug 10, 2018

RichardHaselgrove commented Aug 10, 2018

Ageless93 commented Aug 10, 2018

RichardHaselgrove commented Aug 10, 2018

JuhaSointusalo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharlieFenton left a comment

Choose a reason for hiding this comment

CharlieFenton commented Aug 11, 2018

RichardHaselgrove commented Aug 11, 2018 • edited Loading

RichardHaselgrove commented Aug 11, 2018

RichardHaselgrove commented Aug 11, 2018 • edited Loading

Ageless93 commented Aug 11, 2018

JuhaSointusalo left a comment

Choose a reason for hiding this comment

JuhaSointusalo commented Aug 11, 2018

RichardHaselgrove commented Aug 11, 2018

RichardHaselgrove commented Aug 12, 2018

CharlieFenton commented Aug 12, 2018

CharlieFenton commented Aug 12, 2018

AenBleidd commented Aug 12, 2018

RichardHaselgrove commented Aug 13, 2018

AenBleidd commented Aug 10, 2018 •

edited

Loading

RichardHaselgrove commented Aug 11, 2018 •

edited

Loading

RichardHaselgrove commented Aug 11, 2018 •

edited

Loading