arohl / gdis

A visualization program for the display, manipulation, and analysis of isolated molecules and periodic structures
GNU General Public License v2.0
43 stars 17 forks source link

Model editing additions #44

Closed cajfisher closed 3 years ago

cajfisher commented 3 years ago

Enable editing of element type separate to atom name Enable editing of site occupancy factors Add regions panel to Model Editing dialogue Other minor fixes

ovhpa commented 3 years ago

Hello,

I found 2 little things in this PR.

First of all static analysis report the following minor BUG:

Description: Access to field 'data' results in a dereference of a null pointer (loaded from variable 'list')
File: gui_edit.c
Line: 3144

The function is gui_refresh_selection within the switch statement. It seems that in core = list->data; the data element of list can be access while list was not test for NULL.

I think the n = g_slist_length(list); statement should prevent this, given that it is defined as:

guint g_slist_length (GSList *list)
{
  guint length;
  length = 0;
  while (list)
    {
      length++;
      list = list->next;
    }
  return length;
}

Which will fortunately return 0 on list==NULL, or a positive number in the other case.

So there should be no problem.

BUT, if we want to silence the static analysis, the switch statement (which only consist in 3 case as -1 can't be reached, and 1 case is a simple break) can be equivalently rewritten as:

list = model->selection;/*unchanged*/
if(list){
  n = g_slist_length(list);
  if (n==1) core = list->data;
  else {
    for (list=model->selection ; list ; list=g_slist_next(list))
      {
        core = list->data;
        ARR3ADD(centroid, core->x);  
        q += atom_charge(core);
        m += atom_mass(core);
        if (core->has_sof)
          s += core->sof;
        else
          s += 1.0;
      }
    s /= (gdouble) n;
    VEC3MUL(centroid, 1.0 / (gdouble) n);
    /* special print case - centroid display */
    cflag = TRUE;
    core = NULL;
  }
}

What is your opinion?

Secondly, outside of static analysis, I found a minor inconvenient: when I modify some field of the "Model: Edit" and no atom is selected, then nothing happens (which is the expected behavior). However, if some atom was selected before, it seems that one atom will be modified (and I'm not sure which one) even though no atom is selected. I found this when modifying some fields and unselecting atoms by mistake.

To be more clear:

How to reproduce: select some atoms, change atom element (example). Then unselect all atoms, and make another change: one atom will be modified. Expected behavior: when no atom is selected, no modification can happen.

Other than that everything else is OK.

Best,

cajfisher commented 3 years ago

Thanks for tracing this.

Actually, we can do slightly better with the gui_refresh_selection code. In the earlier version I basically tacked on the my changes to Sean's original code (not sure why the n == -1 was needed), so it was a bit clunky. I've reworked it now based on your suggestions with a bit more streamlining.

Also fixed the bug that allowed unselected atoms to be modified!

Best,

ovhpa commented 3 years ago

Pass the gcc / clang / icc and static analysis / libasan / valgring tests ;)

I'm merging. BTW, did you get the bug where background and other colors can't be changed and can't be saved?