Fix for issue #676: title field not seem to be working with JSON profile

[rkashinin]

No description provided.

[nautics889]

Verified on Ubuntu 22.04, it works correct, the tab has it's title. Thank you for contributing!

---

However there are several points remaining unclear for me.

The function from_json() accepts two arguments layout_name and json_name (default None). As i understood the purpose of json_name if to distinguish the name of property in layout from the actual key provided in JSON. Can they be different anyway? In your implementation there aren't calls from_json() passing json_name as an agrument (so json_name would equal to layout_name all the time). Does json_name really matter?

Also i'm a bit confused about using __setitem__() instead of children[parent + "." + str(order)][layout_name] = layoutjson[json_name].

I'd suggest the next implementation:

    def build_terminal_layout(self, layoutjson, children, parent, order):
        dbg ('Building a terminal from json: %s' % layoutjson)

        children[parent + "." + str(order)] = {
            'type': 'Terminal',
            'order': order,
            'parent': parent,
            'profile': self.profile_to_use
        }
        for item in ('command', 'title'):
            if item in layoutjson:
                children[parent + "." + str(order)][item] = layoutjson[item]

or even like this (a bit prettier as for me):

    def build_terminal_layout(self, layoutjson, children, parent, order):
        dbg ('Building a terminal from json: %s' % layoutjson)

        child_conf = {
            'type': 'Terminal',
            'order': order,
            'parent': parent,
            'profile': self.profile_to_use
        }
        for item in ('command', 'title'):
            if item in layoutjson:
                child_conf[item] = layoutjson[item]

        children[".".join([parent, str(order)])] = child_conf

(I've checked the code above, it works correctly)

Please, let me know if i'm wrong and we should consider cases when the name in a layout conf and the actual JSON key (meaning json_name really matters).

[nautics889]

@rkashinin

[rkashinin]

Thanks for the review,
I think that you are right and in this situation json_name doesn’t matter, but there is no guarantee that json_name and layout_json always equal. That’s why without json_name now, it will be much more difficult to change the program later.

From my point of view it's a matter of design but the setitem function sets the element in the dictionary. So I don’t really understand, why do you prefer square brackets?

[nautics889]

@rkashinin

---

I think that you are right and in this situation json_name doesn’t matter, but there is no guarantee that json_name and layout_json always equal. That’s why without json_name now, it will be much more difficult to change the program later.

Sounds reasonable.

---

As for __setitem__(). There are several way to set item for specific key in dictionary: subscript notation, using update() method and using __setitem__() method (like you've used).
Meanwhile calling of __setitem__() entails poor perfomance time comparing to subscript notation:

>>> import timeit
>>> using_brackets = """
... bar = {}
... bar['foo'] = 'foo'
... """
>>> using_setitem = """
... bar = {}
... bar.__setitem__('foo', 'foo')
... """
>>> print("Brackets: ", timeit.timeit(using_brackets, number=10000))
>>> print("__setitem__(): ", timeit.timeit(using_setitem, number=10000))
Brackets:  0.000403950998588698
__setitem__():  0.0008579820023442153

(Python 3.8.10 \n[GCC 9.4.0])

Not to mention it's essentially less readable form of doing the same thing. Similar points are described at this topic on SO.

Futhermore, Python's special method __setitem__() is supposed to implement behaviour when using subscript notation:

class CustomDict(dict):
    def __setitem__(self, k, v):
        print(f'Set value "{v}" for key "{k}"')
        super().__setitem__(k, v)
        
>>> mydict = CustomDict()
>>> mydict['bar'] = 'foo'
Set value "foo" for key "bar"
>>> print(mydict)
{'bar': 'foo'}

(Python 3.8.10 \n[GCC 9.4.0])
So in this case it looks like unreasonable calling of Python's special method.

[rkashinin]

0001-Fix-for-issue-676-title-field-not-seem-to-be-working.patch
0002-Fix-for-issue-676-title-field-not-seem-to-be-working.patch
Thanks for your explanation,
According to your review I’ve made some changes and I send you patch files with them.
Is it ok, or should I make another pull request?

[nautics889]

@rkashinin thanks for the update!

You can just upload a new commit in a top of the current. Then it's going to be automatically updated here, in the current PR.
So, it would be like this:

1. Open your project, ensure you're in the right branch (issue-676 in your case): git checkout issue-676;
2. Make changes to code;
3. Make a new commit: git add terminatorlib/configjson.py, then git commit and a message like minor fix;
4. Upload then: git push -u origin issue-676;

(Note: the argument "origin" in 4th step may be different depending on which settings are in your .git/config file; to clarify you can run git remote -v and chose the name which corresponds to URL of your fork on GitHub)

---

Alternatively you can create a new pull request, of course, it's up to you, choose the easier option :)

---

Hope this help!

[rkashinin]

Thanks for your advice,
I wasn’t sure about what to do, couse I’m a new one on git hub.
You helped a lot!