Streamline templates/views that share context variables

[susanodd]

Goal Reduce Redundancy in:

• All views and templates

• CSV routines

• Display routines

• GlossListView

• SenseListView

• MorphemeListView

• GlossDetailView

• admin_senses_list.html

• admin_gloss_list.html

[vanlummelhuizen]

The views mentioned above share a lot of code. The same goes for the templates. There are probably more places with a lot of overlapping code.

Moreover, the methods in the views are very, very big.

By taking pieces of code out of those views and templates and placing them in a separate function or method of a parent class, we end up with smaller units of code that is shared instead of copied. This makes the code easier to understand and maintain.

[susanodd]

I'm starting out by making separate get_context_datafor ... functions in a new python file.

There is an extreme amount of duplication in the GlossListView context function!! And the various variables get set multiple times in the code. This makes it impossible to see any dependencies.

[susanodd]

@vanlummelhuizen I would prefer to merge the modified code as I progress. Otherwise there is a chance that others will make modifications to the old code and make it difficult to merge.

[vanlummelhuizen]

I would prefer to merge the modified code as I progress.

You probably need to coordinate with @Jetske and her work in other branches.

By the way, @Woseseltops and I are not working next week because of herfstvakantie.

[susanodd]

This is an example of what I was trying to say, this setting SHOW_DATASET_INTERFACE_OPTIONS

In the context variables, it checks to make sure it exists as a setting, otherwise it sets it to False. https://github.com/Signbank/Global-signbank/blob/682e52dea2d22d374be3f91f6b4421f711c5b579/signbank/dictionary/adminviews.py#L382-L385

However, this could just be replaced by settings.SHOW_DATASET_INTERFACE_OPTIONS in the template and not use a context variable.

For example, as is done here:

https://github.com/Signbank/Global-signbank/blob/682e52dea2d22d374be3f91f6b4421f711c5b579/signbank/dictionary/templates/dictionary/gloss_detail.html#L970

There is a test that checks settings.

I moved lines similar to this into a new context data function that primarily fills in context needed for the mega-gloss search form.

[susanodd]

There are extensive lines of code to set up the glosssearchform for the template. Part of this is due to all the multi select field choice fields and the use of colours. These definitely can go in a separate function. There is also numerous code for setting up all the phonology fields since some need three columns. Perhaps it’s possible to have a separate template for the different search panels. In the query set method there is also lots of testing to find out which fields are filled in. Lots of searches involve multiple related models. There are also multilingual searches.

[susanodd]

The columns are quite crowded for sentences when more than 2 languages are shown. (Fixed.)

[susanodd]

This has been merged and deployed.

But the streamlining continues....

[vanlummelhuizen]

@susanodd Nice work cleaning things up.

I see the functions in signbank/dictionary/context_data.py are still pretty long. Do you plan to work on this, or do you want me to do that? I have some ideas.

[susanodd]

If you have some ideas, please go ahead!

I was considering separating the giant GlossSearchForm into separate forms. But it’s not clear how many other places would be affected.

[susanodd]

@vanlummelhuizen I added two new functions to query_parameters.py

The goal is to condense all the "get" processing of the form fields during the query.

The original code (adminviews.py, get_queryset, MorphemeListView) processes in order all of the different possible get arguments. In the new function, I have swapped around the loop so it loops over the get fields and then gets type information from the form base fields, like this: https://github.com/Signbank/Global-signbank/blob/90128e0ce94206624a3b3b1195c87ca2df144e17/signbank/query_parameters.py#L562

For the language fields, Annotation, Lemma, Translations (Senses), these fields are only created during the __init__ method of the form. So they are not available statically.

[susanodd]

RESOLVED Made these be fields instead of attributes.

Okay, so I added a "self.search_form" to be able to see the search form inside of get_queryset.

However, the "language" fields are not actually fields of the form. They are set up as "attributes" inside of the __init__ method:

https://github.com/Signbank/Global-signbank/blob/90128e0ce94206624a3b3b1195c87ca2df144e17/signbank/dictionary/forms.py#L491-L495

whereas other fields are set up as fields: https://github.com/Signbank/Global-signbank/blob/90128e0ce94206624a3b3b1195c87ca2df144e17/signbank/dictionary/forms.py#L502-L503

This has the consequence that they can't be iterated over from the perspective of the form. They're only visible by doing searchform.__dict__

[susanodd]

RESOLVED Hmmm. I've rewritten the (morpheme) search form to make them "fields" instead of attributes. However, now the "label" is not visible when the page is loaded. The self.search_form needs an initial value, but it is initially None because the context variables (dataset languages) need to be known before the form can be initialised.

Probably the view needs an __init__ method to set up the search form (labels) before the template needs to be displayed. Nope, that's not possible, since the selected datasets are in the request and there is no request yet.

[susanodd]

RESOLVED This is a bit of chicken or egg. The "languages" need to be known in order to display the labels in the search form. But they are not known until the form is created. At the moment, they are initialised and visible (in print statements inside of __init__ but these labels are not known when the template is displayed because they are dynamic.

When the form is initialsed, the languages are an argument. In order to make this not need the languages (to be computed in order to create the self.search_form variable), then the fields can be done for "all" languages, then just ignore, not show, the unused ones in the template.

[susanodd]

I made the language fields of the (morpheme) search form be fields instead of attributes. To support this, template filters were modified to retrieve the label. A duplicate "Annotation Instructions" (useInstr) field was fixed in the settings. In various search forms it was appearing in multiple panels. The language get fields are now processed in the queryset_from_get function in query_parameters.py

[susanodd]

I reduced the search form / get processing code for glosslistview and senselistview as for morphemelistview.

[susanodd]

TO DO:

• Clean up the context variables in MorphemeListView, MinimalPairsListView, LemmaListView

[susanodd]

@vanlummelhuizen perhaps you know an answer.

There is a hidden input field <input id="menu_bar_search_mirror" type="hidden" name="search" class="form-control" value="{{ menu_bar_search }}"/>

This is in both GlossListView template and SenseListView template. In the former, it is useful and gets filled with the search value from the menu bar.

But the SenseListView, the menu bar once filled in, it is redirected to /signs/search/ and does not contribute to the search of senses. (I removed the code 'search' field code from get_queryset of SenseListView there because it was not reachable.)

(The menu_bar_search_mirror code is very old. It has endured many evolutions.)

This menu_bar_search attribute is included in multiple search forms. This is partly to make them view-independent as the code can just check if the attribute is there, to get the label from the attribute (QueryListView gets labels this way).

Question is: Can any of this be removed, except for that of GlossSearchForm and GlossListView ?

The actual "form" for the menu bar search is inside the menu.htmltemplate and the javascript code that does stuff with the menu fields is in baselayout.html. The mirror really is a mirror. The get_queryset gets the values from the url.

[susanodd]

My my my. I put exclamation points at the end of that last -mcomment and look what it did. Hmpf.

[vanlummelhuizen]

My my my. I put exclamation points at the end of that last -mcomment and look what it did. Hmpf.

It is your shell (probably Bash) doing that: https://dev.to/sanixdarker/bash-and-exclamation-marks--3ii8

[susanodd]

My my my. I put exclamation points at the end of that last -mcomment and look what it did. Hmpf.

It is your shell (probably Bash) doing that: https://dev.to/sanixdarker/bash-and-exclamation-marks--3ii8

Yes, I guess so. I was surprised!

[vanlummelhuizen]

Question is: Can any of this be removed, except for that of GlossSearchForm and GlossListView ?

From what you tell me and a quick verification it can be removed. I guess when you worked on SenseListView you copied a lot of code from admin_gloss_list.html to admin_senses_list.html while not all that code was relevant. See https://github.com/Signbank/Global-signbank/commit/5b6b80928cf6ba58ae22b2fa638e9b8f8392ced3

[susanodd]

Question is: Can any of this be removed, except for that of GlossSearchForm and GlossListView ?

From what you tell me and a quick verification it can be removed. I guess when you worked on SenseListView you copied a lot of code from admin_gloss_list.html to admin_senses_list.html while not all that code was relevant. See 5b6b809

Good point! I just found some other code that seems unused in the ajax call that fills in the rows of the SenseListView. (It helps that PyCharm shows the variables dull - not used after declaration.)

[susanodd]

UPDATE: SKIP THIS ONE. RESOLVED self.show_all coded as such. Removed irrelevant uses of it.

@vanlummelhuizen I'm working on condensing the variables in get_queryset in the various views.

Did you notice that in GlossListView there is some weirdness with the show_all variable? There is a self variable, but also a non-self variable, as well as a session variable. The show_all is also a parameter in the django url.

I've condensed them so far as you have done:

class GlossListView(ListView):

    model = Gloss
    paginate_by = 100
    only_export_ecv = False
    search_type = 'sign'
    view_type = 'gloss_list'
    web_search = False
    show_all = False

    def get_queryset(self):
        get = self.request.GET

        show_all = self.kwargs.get('show_all', False)
        self.search_type = get('search_type', 'sign')
        setattr(self.request.session, 'search_type', self.search_type)
        self.view_type = get('view_type', 'gloss_list')
        setattr(self.request, 'view_type', self.view_type)
        self.web_search = get_web_search(self.request)
        setattr(self.request, 'web_search', self.web_search)

        if show_all:

So why is self.show_all ignored?

I suspect it had something to do with that get_queryset is called first and the self variable was not correct? Maybe put it into init or one of the other setups? Then use the self variable? (It just looks weird and confusing as it is.)

In the context_data functions, the "view" is passed as listview but it's only used read-only to make the context dictionary. Its self variables are not assigned to. Ahha. Nope. the Result of the functions is a context. They can't have a side effect on the listview object, unless it is also passed as a result.

[susanodd]

RELEVANT: WHAT IS GOING ON HERE?

@vanlummelhuizen @Woseseltops In looking for unused code, I noticed that there is no ECV button in GlossListView.

What's going on here? Is this a button only ASL uses?

https://github.com/Signbank/Global-signbank/blob/2fa094c47c9a042637c25b68be5fcbd73ba1cb15/signbank/dictionary/templates/dictionary/admin_gloss_list.html#L881-L885

[susanodd]

I spawned a new branch streamline_plus to continue working on this issue using upgraded Python and Django.

[susanodd]

GlossDetailVIew get_context_data is a mess!!!! Likewise, the gloss_detail.html template.

@vanlummelhuizen any insight into how to import "sub-templates" ?

For Senses in GlossDetailView, there is "one" row (<tr> element) that is 400 lines long!!

There is a "mock template" for the scroll bar. But in reality, it's merely javascript in a html file. dictionary/search_result_bar.html

I tried to create a sub-template for the query parameter modal, but because there are "trans" Django elements, it would not allow it to be imported.

[susanodd]

Repaired some morpheme descriptions that were behind other things in the GlossDetailView. (To do so, condensed a huge block of template code that included repetitive calls to the same methods).

[susanodd]

I simplified a bunch of code in the gloss row Ajax code by making use of (new) Gloss display functions for Gloss relational fields. These are then programmatically called inside a loop over the search field names. (This needs to be applied elsewhere in the code.)

An improvement (shown in comments) would be to rename the search fields to match the related_model names in order to reduce the loop over field names (related model names) further. (Something similar was considered before by @vanlummelhuizen for field names in settings, to reduce the code of field choices.)

[susanodd]

Regarding the display functions, I got stuck at the code in #1101

I have no idea what this code is supposed to compute. All combinations of accessing data yield no results. In spite of filling in all the Gloss Morphology fields and having all the data filled in for two (tstMH) morphemes.

I repaired some code that was allowing users to choose NGT morphemes so that the repaired morpheme lookahead only now works for the selected dataset.

Could this bug (that the code gets nothing) be related to datasets? (The live server is currently broken because you can choose morphemes that are not in your dataset there.)

[susanodd]

I made some @Woseseltops inspired improvements: the warnings code of object retrieval and permissions checking in the various get methods of the views has been moved to a single show_warning function. 200 lines of code reduction!