Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lime-app: update title in lime-app #926

Merged
merged 1 commit into from
Dec 9, 2022

Conversation

a-gave
Copy link
Contributor

@a-gave a-gave commented Jul 1, 2022

Append a sed command to lime-apply that update the tag <title> in /www/app/index.html
to allow users to save the lime-app page of their device as a bookmark in the browser, without having to manually override the default title Lime.

I thought it was better to use a single sed command through an uci-default script instead of adding another dependency to lime-app itself, like https://www.npmjs.com/package/react-helmet to manage headers.

@ilario
Copy link
Member

ilario commented Sep 12, 2022

Hi!
This looks very useful!!
Did you test it on a router?
At a first look I am puzzled by ${hostname}... Do you want to get the output of the hostname command or to get the value of a variable? For the former, you should use $(hostname) instead.

@a-gave
Copy link
Contributor Author

a-gave commented Sep 12, 2022

Hi,
I have tested it on a ubnt litebeam m5 and it seems to works fine
Yes ${hostname} is the var taken from
https://github.com/libremesh/lime-packages/blob/master/packages/lime-system/files/usr/bin/lime-apply#L3

I previously tried to do it inside the react/preact app but it didn't seem possible..
And I think I understand the puzzly in appending a sed command like this.
Initially instead of appending it at EOF, I place it in the first section of lime-apply (the one defined by echo "Apply hostname"
) after L4, through a $(sed -ne "/proc/sys/kernel/hostname/r [...]), but I thought it could be risky if this reference will change in the future.

I noticed however that appending the command at EOF, the lime-apply will run a command related to the title after the section defined by echo "Reload routing protocols"
So I thought it could maybe be lighter with an additional explicit echo command like in the code below at L4(instead of the actual simple comment)

  #!/bin/sh
  cat << EOF >> /usr/bin/lime-apply

  echo "Update the title of lime-app" 
  [ -f /www/app/index.html ] && {
          sed -i -e "s/\(<title>\).*\(<\/title>\)/\1\${hostname}\2/g" /www/app/index.html
  }
  EOF
  /usr/bin/lime-apply
  exit 0

Recently, I tried the updated lime app and see that now the title is "LimeApp", so I thought to retire this pr
But if it could be useful anyway, it is also to notice that this script triggers a /usr/bin/lime-apply at first config (to apply the initial hostname). And, I don't want to have misjudged if it could represent a problem for other packages.

@ilario
Copy link
Member

ilario commented Sep 15, 2022

I will test this as soon as I can, but it looks good to me.
If you want to add the echo command it would be better, but in my opinion this could be merged also without it.

@G10h4ck
Copy link
Member

G10h4ck commented Nov 28, 2022

@selankon could this still be useful or should we close the PR ?

Copy link
Member

@ilario ilario left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested on a router, the title now corresponds to the hostname. Ok for me to merge.
Is there an option to edit the hostname from lime-app?
In this case, does lime-apply get executed?
Anyway I think is ok to merge.

@G10h4ck G10h4ck merged commit c9aa653 into libremesh:master Dec 9, 2022
@G10h4ck G10h4ck added this to the mesh-wide milestone May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants