Add more explanation to public view related code

[Woseseltops]

When reviewing #1419 , I noticed there was a lot of code related to searching and 'dynamic languagues', which left me confused. As an example of a frequent piece of code that confused me:

if not self.search_form.is_bound:
    # make sure the form has the dynamic language fields that require the request
    set_up_language_fields(LemmaIdgloss, self, self.search_form)

Why is the search form relevant to the public view? What does it mean for a search from 'to be bound'? What are dynamic language fields? What do they need to be set up, but only when the search from is bound?

Perhaps we can use this issue to

1. Better understand what is going on here
2. Create 1-2 sentence exlanation of this
3. Add this as a comment to the relevant parts in the code

[susanodd]

It's relevant to all views.

It's in that pull request because that's where it was encountered. (Otherwise I need to yank out the code and make yet another pull request with just that code. I already did that with the selected datasets so it makes use of the session variable, which is the only way for public view -- see pull request #1425, as well as repairs to protected media so anonymous users can view the new kinds of video -- see pull request #1435 -- the branch was created before the issue was created, it was fixed for the public view, then the code extracted to make another pull request)

If the form hasn't been initialized with the languages yet, the search does not work yet. (The language fields are not in the form. They are put in the form once they are in the request from the selected dataset languages. Everything used to end up defaulting to NGT and it was not possible to browse in Kata Kolok public glosses because the languages had been set to Dutch and English, as for NGT. It couldn't load the page because it had the wrong languages in the form.)

it's a result of cascading bugs related to the selected datasets getting set to NGT. For public view, the dataset selected is in the session variable. But that was not being used in the original code. (I was working on that together with @vanlummelhuizen last week.)

This has something to do with the order of evaluation of Django when it loads a view. The get_queryset is done before the get_context_data is done. The language fields need the "request" argument, but it is not available in the __init__ method (I had to look up all of that in the Django documentation in order to debug it. The language fields were not being put in the form. So I moved the "dynamic initialization" to put them in the form inside of get_queryset because that is done first -- of the methods we define)

The normal list views have a big "form your query" area at the top. Initially no results are shown.
That has the effect that by the time you type in your query the form has been initialised. Or rather, lots of control flow in the different methods ends up getting the form initialized. Plus it kept defaulting to NGT so it didn't need the request if it had a dataset.

[susanodd]

I will add comments!

I can see from my explanation, I understand what is going on with the code, but I can't describe it concisely.
I will put comments at the appropriate points in the code.

I really don't want to extract the code for yet another pull request.

There is likely going to be another issue about the other session variables. For search type. Recently a new kind of search type was added for annotated sentences. (I removed the text about that because it was distracting.)

[vanlummelhuizen]

What does it mean for a search from 'to be bound'? What do they need to be set up, but only when the search from is bound?

form.is_bound is a Django thing. See https://docs.djangoproject.com/en/5.1/ref/forms/api/. In short: if a form has data, it is bound (to that data), else it is unbound and may be used to create a blank form.

---

For what I understand, the set_up_language_fields function changes the search form according to language and/or dataset choices that are in the user session, i.e. the function adds language specific fields. All clear and OK for me.

The get_queryset method may not be the ideal location to do this, however. While looking at the list under heading Method Flowchart on https://docs.djangoproject.com/en/5.1/ref/class-based-views/generic-display/#listview, I see two possible locations:

1. setup if you want it to be done early in the process (since __init__ does not receive the request)
2. get_context_data. All things that are used in the template are put in the context here, including forms.

---

At the time of writing I see 6 lines of comments for each instance of the code @Woseseltops mentions. E.g.

Global-signbank/signbank/dictionary/adminviews.py

Lines 367 to 374 in da05c74

	if not self.search_form.is_bound:
	# if the search_form is not bound, then
	# this is the first time the get_queryset function is called for this view
	# it has already been initialised with the __init__ method, but
	# the language fields are dynamic and are not inside the form yet
	# they depend on the selected datasets which are inside the request, which
	# is not available to the __init__ method
	set_up_language_fields(AnnotatedSentence, self, self.search_form)

I don't think that is necessary. First, is_bound should be clear to Django-developers (see above). And since set_up_language_fields has some documentation and your IDE (PyCharm, VSCode, etc.) should be able to display that documentation when you hover over the function name in the calling line, we could get rid of these comments all together. At most we could add something like "Add language dependent fields dynamically". Or change the function name to add_language_dependent_fields.

---

Question for @susanodd : is it necessary that the search form is bound for the language dependent fields to be added? I think it could be an unbound form that gets fields added to it based on the dataset selection in the user session. Correct?

[susanodd]

Can you guys fight it out?

I was fine with the original ONE-LINE comment! (See issue description.)

The search form is in the "self" of the views because it needs to be shared by all the methods. (It has been this way for a year or so, because of query parameters and making sure the form stays filled instead of generating a new form. The users wanted to keep seeing their parameters in the form.) It is unbound until the view is actually used. It used to be defaulting to NGT in the original code. But that caused surprise errors for Kata Kolok because the language fields were wrong.

The Django get_queryset is called BEFORE the get_context_data method. I merely moved the initialization method that has been there for 6 months or maybe a year or so to a different method. (Then @Woseseltops asked questions.)

Just yesterday, I had to move the initialization to the front of two views because it wasn't getting initialized yet. (See commits.)
This depends on control flow. You have to actually be using Signbank, not just reading code to see why this is necessary.

I'll happily change it back to the ONE LINE COMMENT. I was happy with that.

[susanodd]

@Woseseltops complained about the ONE LINE COMMENT.

I changed it to be SIX LINES to be pedantic. Because he complained about the ONE LINE.

I'm going to change it back to what it was, just ONE line. I am fine with the original ONE LINE.

[vanlummelhuizen]

Can you guys fight it out?

Sure. I talk to @Woseseltops on Thursday.

I'm going to change it back to what it was, just ONE line. I am fine with the original ONE LINE.

Please wait to change things (back) until we settle this.

[Woseseltops]

In short: if a form has data, it is bound (to that data), else it is unbound and may be used to create a blank form.

Ah didn't know, thanks :)

Or change the function name to add_language_dependent_fields.

Even better!

I changed it to be SIX LINES to be pedantic. Because he complained about the ONE LINE.

I detect frustration here, sorry the Signbank project is giving you negative emotions! I appreciate your attempt to improve the understandability of the code!

Having said that, if you read my original request, I'm not asking for more comments to the code immediately, I asked to start with formulating a short explanation together, here in this issue :). Short, for similar reasons I ask you to use the <details> tag.

Would the proposal by @vanlummelhuizen work for you?

[vanlummelhuizen]

Would the proposal by @vanlummelhuizen work for you?

Yes. My suggestion would be to let the short explanation we come up with be reflected in the function name and function docstring. Then, if the function name is still unclear to the reader, he/she can hover it in the IDE to see the docstring. This way we wouldn´t need comments at all.

Rule of thumb: code should be self explanatory, by using good names and docstrings, and with the help of a good IDE.