Ticket #3763 (closed task: fixed)

Opened 8 years ago

Last modified 8 years ago

Introduce run_listbox_with_data()

Reported by: mooffie Owned by: mooffie
Priority: major Milestone: 4.8.19
Component: mc-core Version: master
Keywords: Cc:
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description (last modified by mooffie) (diff)

The problem:

Functions that use run_listbox() can be complicated and breakable, if they need to figure out some pointer given the number of the item selected.

(For example, in the case of edit_window_list() this will make its code break if we change how widgets are Z-ordered --which was the 1st solution I tried for #3760.)

The solution:

A listbox can already store pointers. So we introduce a variant of run_listbox() which returns pointers. We then use it to simplify a couple of functions.

Change History

comment:1 Changed 8 years ago by mooffie

  • Owner set to mooffie
  • Status changed from new to accepted
  • Branch state changed from no branch to on review

branch: 3763_run_listbox_with_data
Initial changeset:7bccfe9b81c0456585aa2a86bda77c793fe9c779

(There are 3 commits here.)

Version 0, edited 8 years ago by mooffie (next)

comment:2 Changed 8 years ago by mooffie

  • Description modified (diff)

comment:3 follow-up: ↓ 5 Changed 8 years ago by zaytsev

LGTM, Andrew?

Minor comment: the pointer WEdit *e can be WEdit * const e unless I'm confused, but that might be my Haskell habits :-|

comment:4 Changed 8 years ago by mooffie

If this ticket gets approved, before I commit I'll change:

if (e != NULL)
    val = e->data;

to:

if (e != NULL) {
    g_assert (!e->free_data); /* Don't return pointers that are about to be deallocated, as in listbox_add_item(..., TRUE). */
    val = e->data;
}

comment:5 in reply to: ↑ 3 Changed 8 years ago by mooffie

Replying to zaytsev:

Minor comment: the pointer WEdit *e can be WEdit * const e unless I'm confused


Could you please elaborate? (I'm sure I'm about to learn new stuff.)

(Haskell... I programmed in it for over a year. It was the language I found the most joyous to use.)

comment:6 follow-up: ↓ 8 Changed 8 years ago by zaytsev-work

If this ticket gets approved, before I commit...

Suggest to commit with --fixup and then, before you merge with --no-ff, just do a rebase -i --autosquash and it will reorder & squash the commits automatically. You probably know about that already, but thought I'd mention it anyways.

comment:7 Changed 8 years ago by andrew_b

  • Votes for changeset set to andrew_b
  • Milestone changed from Future Releases to 4.8.19

comment:8 in reply to: ↑ 6 Changed 8 years ago by mooffie

So, folks, is this "approved"?

(Can I assume when somebody writes "LGTM" that he's voting for it?)

Replying to zaytsev-work:

Suggest to commit with --fixup


Yes, Andrew once explained this to me (I already suqashed the branch: I had the impression it was "approved").

comment:9 Changed 8 years ago by zaytsev

  • Votes for changeset changed from andrew_b to andrew_b zaytsev
  • Branch state changed from on review to approved

LTGM = looks good to me, sorry about that. Yes, sometimes we forget to change the status after adding votes, fixed this just now. I'm afraid that's all I can do for now, need to go to sleep.

comment:10 Changed 8 years ago by mooffie

  • Status changed from accepted to testing
  • Votes for changeset changed from andrew_b zaytsev to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

comment:11 Changed 8 years ago by mooffie

  • Status changed from testing to closed

(Yes, I updated NEWS-4.8.19.)

Sweet dreams!

comment:12 Changed 8 years ago by zaytsev

Could you please elaborate? (I'm sure I'm about to learn new stuff.)

I'm afraid that you'd be left disappointed by my insights :-/ (now that I've finally gotten to it)

All I meant is that in this situation it appeared to me that you don't need the capability to reseat the pointer, and so in such cases I would always use a "type * const" pointer to prevent reseating it by accident. Unfortunately, there are no references in C like in C++ (special syntax for pointers that can't be reseated after initialization), and so it's a bit clumsy to use, but it feels a bit safer.

This where the Haskell reference comes in; as you know, there immutability is ubiquitous and manipulating mutable objects requires explicit opt-in as in choosing a special type, using ST monad and what not. Consequently, after having programmed in Haskell, I feel physically unwell when I have to manipulate mutable objects, and therefore if I have to do C/C++, I tend to use "type const * const" when possible, or, at least, "type * const", when one has to be able to modify the value, but doesn't need to be able to reseat the pointer. Another facet of this paranoia is to use stack when possible and break down code in blocks to declare variables local to the block, limiting the scope to furthest possible extent (in mc sources you will find monster functions that can be so broken down in many blocks, which actually hints at that their McCabe number is way too high, and they have to be refactored anyways, but that's another story...).

There really isn't much more to it, and now, I can't even seem to find the WEdit *e that I was referencing here, which is probably what confused you, so apparently, I was looking at several tickets at the same time, and posted this comment in the wrong ticket. Sorry about that...

Note: See TracTickets for help on using tickets.