Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

Elektra-web 1.2 tasks #1845

Closed
32 of 34 tasks
markus2330 opened this issue Mar 18, 2018 · 20 comments
Closed
32 of 34 tasks

Elektra-web 1.2 tasks #1845

markus2330 opened this issue Mar 18, 2018 · 20 comments
Assignees
Milestone

Comments

@markus2330
Copy link
Contributor

markus2330 commented Mar 18, 2018

elektra-web1.2 brings many nice improvements and features but there is still much to be done. At some places it feels like it was not tested, or not tested with realistic data.

  • should show Elektra logo and not react logo
  • when starting elektrad the used port is printed, but not so for client (seems like it was printed much later?)
  • When connecting to client, there is still the host written in gray, which is very confusing
  • error messages of kdb are written very small and (during startup) are displayed very short
  • if (meta) keynames contain newlines I got the error KDBError: Command failed: kdb getmeta "user/metakey" "h" Metakey not found, see kdb ls -0: regression test missing #1844 for details
  • user/ is not collapsed by default and cannot be collapsed?
  • user/ namespace missing if there is no key in user/ yet
  • "settings" for key is misnamed (should be metadata for key)
  • metadata dialog a bit confusing:
    • section "Type" in the middle but read only and validation is also part of type.
    • No way to hide the "Please note:".
    • Can regex not also lead to loss? -> no, it doesn't overwrite anything
    • "example" should not have "e.g." as example.
  • When changing from number to boolean, boolean is still displayed
  • When duplicating a key and name already exist, something very wrong happens (value moved to section above?)
  • pressing enter often not supported (e.g. giving name for duplicated key)
  • why is config in user even when running client as root? (it is in user/sw/elektra/web/#0/current/instances which is okay, when started as user) (read write from cascading keys should do the right thing) -> issue with yajl inconsistency regarding cascading keys (Elektra-web 1.2 tasks #1845 (comment)) -> use relative keynames in storage and export files #51
  • when creating a key, type cannot be selected
  • for keys which have subkeys: when changing type, and then changing value, the type is gone afterwards
  • read-only keys should not be called "disabled" (this is confusing, people might expect that disabled keys do not configure software)
  • by default "string" is shown as type, although it is actually "any" (or even nothing)
  • keys with umlauts cause problems: "KDBError: Command failed: kdb lsmeta "�sd�y2 = "�[sec�iy1 ct{on`3" /bin/sh: 1: " could not reproduce, please provide kdb export or kdb set command
  • implement other namespaces: dir/ spec/

Later added:

  • The default is currently is not shown, which is also a usability problem.
  • It seems like your tool currently does not distinguish between no value and no key.

conceptual problems/questions

  • unclear when something is done (e.g. large key set loaded), some feedback would be great (blue cycle not always there)
  • README.md inconsistent, not up-to-date, incomplete (e.g. how to start client on different ports with Elektra and so on).
  • how is it visible if a key (with subkeys) is visible at all

visibility of keys

  • add two fields for instances: "visibility" and "description" (a user might add the same computer twice with advanced and basic configuration, description is needed to remember what an instance was for)
  • only show keys which are more (or equal) visible than selected for the instance
  • allow visibility for a key to be selected in meta data dialog

leftovers of #1759

  • custom field to create ranges (instead of putting them in as a string)
  • please avoid special syntax to specify enums (please add some interface where new valid enum entries can be added). Please also use new syntax (check/enum/#). And make sure to change check/type accordingly when enum is set.

cmake integration

  • cmake integration missing (run npm install while doing make install) (copied from elektra-web #1314)

positive things

  • 👍 search is now much improved
  • 👍 nice non-intrusive menus
  • 👍 Firefox works
  • 👍 arbitrary meta data checks for already handled meta data

non-goals

to keep the effort from exploding, following tasks are not longer the scope of elektra-web:

  • cluster configuration
  • proper nodejs binding (not using kdb CLI tool)
@markus2330 markus2330 changed the title elektra-web1.2 Elektra-web 1.2 tasks Mar 18, 2018
@markus2330 markus2330 added this to the 0.8.23 milestone Mar 18, 2018
@markus2330 markus2330 mentioned this issue Mar 18, 2018
23 tasks
@omnidan
Copy link
Contributor

omnidan commented Mar 20, 2018

@markus2330

user/ is not collapsed by default and cannot be collapsed?

my idea was that you usually want to modify configuration in user/, so it is always expanded. I think otherwise the namespaces could be confusing and the user might click on system/ instead of user/

dir/ spec/ namespace missing? (and even user/ if there was no key in user/)

I don't think elektra-web should be the "main" editor for elektra. It should be used to edit configuration if you are not familiar with elektra at all. This also means that the config is already set up in user/ (e.g. by an administrator). I can add all namespaces, but I think it will be more confusing for users (which is also why originally I was only showing the user/ namespace).

keys with umlauts cause problems: "KDBError: Command failed: kdb lsmeta "�sd�y2 = "�[sec�iy1 ct{on`3" /bin/sh: 1: "

I couldn't reproduce this. I tried adding keys and metakeys with umlauts and there were no issues for me. Did you add the keys with umlauts via the web UI or through a kdb command?

how can I change types etc.. in spec/ namespace?

As I said before, I don't think other namespaces should be the scope of this project.

unclear when something is done (e.g. large key set loaded), some feedback would be great (blue cycle not always there)

the loading spinner should always be there when there are pending requests. I noticed that for large key sets it takes a while for the spinner to appear, I will look into this issue.

how is it visible if a key (with subkeys) is visible at all
configuration of client not visible within elektra-web?
When changing from number to boolean, boolean is still displayed

what do you mean?

not possible to copy&paste values?

as discussed in person, you can duplicate and drag & drop to achieve the same effect

if there is a spec key, its meta data should also apply to the keys with the same name in other name spaces, this does not seem to be the case.

does kdb lsmeta return those keys? if yes, it should also work

cmake integration missing (run npm install while doing make install)

I still don't really understand what the cmake integration should be doing. I mean, do we really want to install all elektra-web dependencies every time we build elektra? what is the point of that?

visibility of keys

I think this could be an interesting feature, but I really don't want to extend the scope of the project at this point. Maybe add this to an issue with future ideas for elektra-web?

No way to hide the "Please note:".

I wouldn't add this, as then someone could accidentally override data or forget that this happens.

"example" should not have "e.g." as example.

I prefixed all hint texts with e.g. to make it more clear that they are just giving an example, and it is not an actual value entered in the text field. this includes the hint text for the example field. although I can also remove all the e.g. prefixes again (for the hosts field, I removed the hint text completely now because it was too confusing)

@omnidan omnidan mentioned this issue Mar 20, 2018
5 tasks
@omnidan
Copy link
Contributor

omnidan commented Mar 20, 2018

why is config in user even when running client as root? (it is in user/sw/elektra/web/#0/current/instances which is okay, when started as user) (read write from cascading keys should do the right thing)

I tried using cascading keys here but it results in merge conflicts when using export/import:

Command failed: echo '[{"id":"ef5ad47d-6228-4c62-8859-ec17ad0402fa","name":"test","host":"test"}]' | kdb import "/sw/elektra/web/#0/current/instances" yajl↵2 conflicts were detected that could not be resolved automatically:↵/sw/elektra/web/#0/current/instances/#0/host↵ours: , theirs: ↵↵/sw/elektra/web/#0/current/instances/#0/id↵ours: , theirs: ↵↵Merge unsuccessful.↵

You can try this yourself by overwriting the ROOT_PATH env variable with a cascading key (e.g. /sw/elektra/web/#0/current)

@markus2330
Copy link
Contributor Author

@omnidan Thank you for your answers!

my idea was that you usually want to modify configuration in user/, so it is always expanded. I think otherwise the namespaces could be confusing and the user might click on system/ instead of user/

I think we need to trust that users learn the meaning of namespaces.

The namespace "user" often is not the right choice, in particular if system administrators use your tool.

I don't think elektra-web should be the "main" editor for elektra. It should be used to edit configuration if you are not familiar with elektra at all.

That is a very good thought and I fully agree.

Did you add the keys with umlauts via the web UI or through a kdb command?

The keys were already there, it is easily possible that the web UI cannot produce this problems. But you should also test the interaction of any configuration in Elektra and your tool.

I can add all namespaces, but I think it will be more confusing for users (which is also why originally I was only showing the user/ namespace).

I think it is even more confusing if the CLI is needed to create the first key of a namespace :-)

If you want to make it user friendly, you could add some information about what the individual namespaces mean, e.g. when you hover above the namespace. (Or show it like the description is shown.)

As I said before, I don't think other namespaces should be the scope of this project.

Other namespaces can certainly not be beyond scope, but for the feature that specifications are considered everywhere we could argued that it is part of the elektrad rewrite. But honestly the scope is already reduced a lot: e.g. export/import/mounting is also out of scope. The solution is also very simple: If you always use cascading lookups, the metadata is always appended. But the CLI is obviously not the right choice for what you are doing, which creates these troubles.

how is it visible if a key (with subkeys) is visible at all

At the moment (it seemed to me) that your GUI does not show a difference if a key (with subkeys) is present or not present (or visible and not visible).

For example:

kdb set user/a/b/c

web UI would show a and b without giving information that they actually do not exist?

configuration of client not visible within elektra-web?

I mean the configuration of your tool itself (stored in user/sw/elektra/web/#0/current). Maybe it was just an oversight because initially I expected the config to be in system. Does it work if you update these values with the web UI?

not possible to copy&paste values?

It is about copying the values in the text fields. Simple marking of text does not work here.

does kdb lsmeta return those keys? if yes, it should also work

Only if you do cascading lookups, which obviously collides with the requirement to show all namespaces individually. But we could add the -n feature to all CLI programs you use.

I still don't really understand what the cmake integration should be doing. I mean, do we really want to install all elektra-web dependencies every time we build elektra? what is the point of that?

Did you look at the integration of Marvin? It should happen during "make install". The point of that is that all tools can be installed the same way and no manual steps are required for distributors that want to distribute your tool as part of Elektra's packages.

I wouldn't add this, as then someone could accidentally override data or forget that this happens.

I agree but I think the current solution takes too much of the precious space.

e.g. prefixes again (for the hosts field, I removed the hint text completely now because it was too confusing)

What about showing the default value (which is the same as having no value) and have a button to copy the example value into the field? The default is currently is not shown, which is also a usability problem.

It seems like your tool currently does not distinguish between no value and no key.

I added the last two points in the tasks above.

@omnidan
Copy link
Contributor

omnidan commented Mar 21, 2018

@markus2330 I tried adding a key to the spec namespace:

> kdb set spec/test
Create a new key spec/test with null value

but then I got the following error:

> kdb ls /

Sorry, I crashed by the signal SIGSEGV
This should not have happened!

Please report the issue at https://issues.libelektra.org/
fish: 'kdb ls /' terminated by signal SIGABRT (Abort)

it is also not possible to recover from this error, it happens on all further commands

@omnidan
Copy link
Contributor

omnidan commented Mar 21, 2018

I fixed the issue by removing the /usr/local/Cellar/elektra/0.8.21/share/elektra/specification/default.ecf file

@omnidan
Copy link
Contributor

omnidan commented Mar 21, 2018

Thank you for explaining the issues!

I checked all tasks that are already done and moved certain tasks to #1849 (for a future 1.3 release). I really want to get the project done soon so I can still do the usability test and finish my thesis this semester. I will still be fixing all tasks in this issue until the end of this week.

The namespace "user" often is not the right choice, in particular if system administrators use your tool.

I understand that, but proc/ dir/ and spec/ are special namespaces and they would have to be handled differently to ensure good usability. This is why I want to focus on the user/ and system/ namespaces in elektra-web. It wouldn't be hard to add the other namespaces, but it would be hard to get them to work right and without causing any issues.

The keys were already there, it is easily possible that the web UI cannot produce this problems. But you should also test the interaction of any configuration in Elektra and your tool.

Can you give me the command to create a key that breaks elektra-web? Even when creating keys with umlauts via the CLI it does not break for me and I'm not sure how to reproduce this.

I think it is even more confusing if the CLI is needed to create the first key of a namespace :-)

I agree, the user/ namespace should always be shown.

It is about copying the values in the text fields. Simple marking of text does not work here.

I just noticed that my fix for this works only on chrome, I will look into fixing this on firefox too.

Did you look at the integration of Marvin?

I will have a look at it and provide a similar integration.

@omnidan
Copy link
Contributor

omnidan commented Mar 21, 2018

@markus2330

why is config in user even when running client as root? (it is in user/sw/elektra/web/#0/current/instances which is okay, when started as user) (read write from cascading keys should do the right thing)

I figured out why cascading keys didn't work on webd. It seems to be the behaviour of the yajl plugin. Without cascading keys, the output of kdb export user/sw/elektra/web/#0/current/instances yajl looks as follows:

[
    {
        "host": "http://localhost:33334",
        "id": "4fb320d2-97f5-405f-a1d8-10cbba645d22",
        "name": "test"
    }
]

But with cascading keys, the output of kdb export /sw/elektra/web/#0/current/instances yajl is:

{
    "user": {
        "sw": {
            "elektra": {
                "web": [
                    {
                        "current": {
                            "instances": [
                                {
                                    "host": "http://localhost:33334",
                                    "id": "4fb320d2-97f5-405f-a1d8-10cbba645d22",
                                    "name": "test"
                                }
                            ]
                        }
                    }
                ]
            }
        }
    }
}

Is there any way to get consistent behaviour with cascading and non-cascading keys here?

@omnidan
Copy link
Contributor

omnidan commented Mar 21, 2018

keys that do not exist have 40% opacity on their label now (so they are a bit greyed out):

screen shot 2018-03-21 at 17 18 32

in this case, user/hello and user/a/b/c exist, but user/a and user/a/b don't

@omnidan
Copy link
Contributor

omnidan commented Mar 21, 2018

It is about copying the values in the text fields. Simple marking of text does not work here.

Unfortunately, I really cannot find a way to solve this in Firefox.

In webkit based browsers, I can do onDragStart={e => e.preventDefault()} on a part of the element (in this case, only the text field) and it prevents dragging the element while still allowing text selection.

In Firefox, however, it seems to also prevent text selection, so there is no way to prevent dragging while still allowing text selection.

@markus2330
Copy link
Contributor Author

Is there any way to get consistent behaviour with cascading and non-cascading keys here?

Yes, by fixing #51 for yajl. But it would only be a first step, you will soon run into other situations what you cannot do with kdb. kdb is not designed for what you are doing. (kdb shell might be "better" suited but it currently only has ksOutput -- which is not json.)

I think it is even more confusing if the CLI is needed to create the first key of a namespace :-)
I agree, the user/ namespace should always be shown.

I do not understand:

  1. how the problem "you cannot create the first key of a namespace in the web-UI " has anything to do with the solution "the user/ namespace should always be shown".
  2. why the user namespace is special for your purposes (should be always shown)
  3. why dir/ is special (should not be shown)
  4. why usability of different namespaces needs differences in behavior? You have your personas and all of them can easily run into situations where they need all namespaces.

I think it would even harm the usability if namespaces are treated differently as it would make everything more complex. So the simplest solution is maybe the best solution: show all namespaces and show them in the same way.

@omnidan
Copy link
Contributor

omnidan commented Mar 21, 2018

@markus2330 I implemented all namespaces now and they also get shown when no keys are available yet. They are also all collapsed and treated the same way now. However, I still have the weird issue where creating a key in the spec/ namespace causes all further kdb commands to crash (I am using latest elektra from master now).

@markus2330
Copy link
Contributor Author

Thank you.

Please open a separate issue about the crashes, ideally with a backtrace. Maybe you ran into #1821. I'll put @tom-wa attention again at this issue, it is a shame we had to release with this possible crash.

(Btw. I am really interested in your view of namespaces. Maybe there are usability concerns I have not considered.)

@omnidan
Copy link
Contributor

omnidan commented Mar 22, 2018

@markus2330 how can I build just a single tool with cmake? I already tested the file by creating a build directory in the src/tools/web directory and running cmake .. there (with some adjustments, like setting the tool variable manually) and it seems to work. I re-used the structure from marvin so it should work fine with the whole setup.

BTW: everything except visibility is done now

@markus2330
Copy link
Contributor Author

how can I build just a single tool with cmake?

The official way is cmake -DTOOLS="your-tool" /path/to/elektra. Running cmake only with your directory is something that could be realized with cmake but I think it is not worth the effort.

@omnidan
Copy link
Contributor

omnidan commented Mar 23, 2018

@markus2330 I built elektra with -DTOOLS="web" and it seems to work fine.

I also implemented visibility, so all tasks should be done. Please check if everything is fine now 😁

@markus2330
Copy link
Contributor Author

I'll check asap but I am afraid it won't be on the weekend.

@omnidan
Copy link
Contributor

omnidan commented Mar 24, 2018

-> #1853

@omnidan omnidan closed this as completed Mar 24, 2018
@markus2330
Copy link
Contributor Author

Some parts are still not implemented, e.g., "readOnly confusing: you get red sign, but it does not say the reason when clicking". I still only get a red sign.

@markus2330 markus2330 reopened this Mar 24, 2018
@omnidan
Copy link
Contributor

omnidan commented Mar 26, 2018

it works in chrome and safari, not in firefox. I'll have to look into this...

@omnidan
Copy link
Contributor

omnidan commented Mar 26, 2018

@markus2330 I'm moving these tasks to the new issue -> #1853

@omnidan omnidan closed this as completed Mar 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants