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

luci-mod-rdash: add new package #7431

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tp82d1
Copy link

@tp82d1 tp82d1 commented Nov 29, 2024

Description

This PR adds a new package, luci-mod-rdash, which provides an interactive dashboard for routers.

Key features:

  • Dynamic icons for internet status, device information and connected clients.
  • Real-time system information display, including CPU usage and connected devices number.

@stangri
Copy link
Member

stangri commented Nov 29, 2024

Not sure if you'd want to change things again, but with the dashes in luci-mod-info, the underscore in info_dashboard looks out of place and might facilitate typing errors when typing the package name.

@tp82d1 tp82d1 changed the title luci-mod-info_dashboard: add new package luci-mod-info-dashboard: add new package Nov 29, 2024
@tp82d1 tp82d1 force-pushed the add-info-dashboard-package branch from 39f8783 to 04e494a Compare November 29, 2024 19:02
@tp82d1
Copy link
Author

tp82d1 commented Nov 29, 2024

Updated the package name from luci-mod-info_dashboard to luci-mod-info-dashboard for consistency as suggested.

@Ansuel
Copy link
Member

Ansuel commented Nov 29, 2024

screenshot of this?

@systemcrash
Copy link
Contributor

@Ansuel

See previous attempt at #7424

screenshot of this?

@tp82d1
Copy link
Author

tp82d1 commented Nov 29, 2024

@Ansuel

Screenshots

Dashboard

dashboard

Internet Status When Connected

InternetStatus_connected

Internet Status When Not Connected

InternetStatus_NotConnected

Device Information

DeviceInfo

Connected Clients

DHCP

@tp82d1 tp82d1 force-pushed the add-info-dashboard-package branch from 04e494a to cc72d2b Compare November 30, 2024 07:59
@systemcrash
Copy link
Contributor

Consider naming your dashboard something creative. Name it after yourself if you want. "info" is waaaaay too generic and may clash with namespaces eventually that shall be used by openwrt itself.

@systemcrash systemcrash marked this pull request as draft November 30, 2024 14:07
@tp82d1 tp82d1 force-pushed the add-info-dashboard-package branch 2 times, most recently from 84f6a85 to 21da814 Compare November 30, 2024 17:52
@tp82d1 tp82d1 changed the title luci-mod-info-dashboard: add new package luci-mod-rdash: add new package Nov 30, 2024
@tp82d1 tp82d1 force-pushed the add-info-dashboard-package branch from 21da814 to b612532 Compare November 30, 2024 17:58
@tp82d1
Copy link
Author

tp82d1 commented Nov 30, 2024

  • Renamed package to luci-mod-rdash for clarity and namespace safety.
  • Replaced minified files with readable versions as requested.
  • Cleaned up commit history and removed PKG_VERSION/PKG_RELEASE fields.

@tp82d1 tp82d1 marked this pull request as ready for review November 30, 2024 17:59
@systemcrash
Copy link
Contributor

squash commits please

@tp82d1 tp82d1 force-pushed the add-info-dashboard-package branch 2 times, most recently from 42fcd2d to 6d0ce68 Compare November 30, 2024 20:58
This commit includes:
- Adding the new package.
- Renaming the package and replacing old files.
- Adding an extra line at the end of files.

Signed-off-by: Rick Clark <tplink.82d1@proton.me>
@tp82d1 tp82d1 force-pushed the add-info-dashboard-package branch from 6d0ce68 to e9e3667 Compare November 30, 2024 21:00
@tp82d1
Copy link
Author

tp82d1 commented Nov 30, 2024

Changes made as requested:

  • Added an extra line at the end of the files.
  • Squashed commits into one commit.

@systemcrash
Copy link
Contributor

Revise your commit message to simply "introduce new luci-mod-rdash dashboard" or something so people don't go looking for any "changes".

Any comments from @dannil or @stangri or @stokito ?

@systemcrash
Copy link
Contributor

One question: how does everything look in dark mode? Those black line diagrams on a dark background might look interesting. :)

Copy link
Contributor

@dannil dannil Nov 30, 2024

Choose a reason for hiding this comment

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

This file is using tab indentation instead of space as the rest of your JS files, you may want to align all the files.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well spotted. Yes please, use tab indentation.

@systemcrash
Copy link
Contributor

One question: how does everything look in dark mode? Those black line diagrams on a dark background might look interesting. :)

Take a look at how I did it in dfd802a (if your css does not already handle that).

# No configure step needed
endef

define Build/Compile
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove the section? As far I remember you may want to declare the empty section to override i.e. remove the default compile step. Please try to run the build without it

# No compile step needed
endef

define Package/luci-mod-rdash/install
Copy link
Contributor

Choose a reason for hiding this comment

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

The Luci build system will do all the steps itself. No need to declare them here.

@@ -0,0 +1,300 @@
.dashboard-container {
Copy link
Contributor

Choose a reason for hiding this comment

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

the cascade.css has variable with standard colors that you may want to use here

@stokito
Copy link
Contributor

stokito commented Nov 30, 2024

tp82d1#1

var mem = L.isObject(memoryinfo.memory) ? memoryinfo.memory : {};
var totalMemory = mem.total || 0;
var usedMemory = (mem.total && mem.free) ? (mem.total - mem.free) : 0;
var availableMemory = mem.free || 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

the var is unused

expect: { result: [] }
});

var MountSkipList = [
Copy link
Contributor

Choose a reason for hiding this comment

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

the list is unused

var boardinfo = data[0],
systeminfo = data[1],
memoryinfo = data[2],
mounts = data[3],
Copy link
Contributor

Choose a reason for hiding this comment

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

the var is unused

@tp82d1
Copy link
Author

tp82d1 commented Dec 1, 2024

Thanks all for the reviews. To be honest, I didn't think it would require lots of these steps (it's my first time). The goal of this project was about "getting everything in one place, just like the default firmware of a TP-Link router dashboard." So I took some parts of "10_system.js, 20_memory.js, 30_network.js, and 40_dhcp.js" and some parts of other packages, such as Luci-mod-dashboard and Luci-app-cpu-status, to combine them in one package. Im not experts in coding, so I wrote the code with ChatGPT's help. Finally, I will try to fix what you mentioned; if I can't, I'll shutdown the project. In the meantime, I'll change the pull to draft.

@tp82d1 tp82d1 marked this pull request as draft December 1, 2024 01:08
@hnyman
Copy link
Contributor

hnyman commented Dec 1, 2024

If this gets merged, I hope that you're committing to maintain this in compatible status for several years.

(We have been burned with enthusiastic creation of some theme or dashboard, and then soon after that the creator loses interest, and the app starts rotting when there are changes in the surrounding OpenWrt. )

"description": "Grant access to the system route status",
"read": {
"ubus": {
"file": [ "exec" ]
Copy link
Contributor

Choose a reason for hiding this comment

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

This exec permissions does not appear to be required, please drop it.

"luci": [ "getVersion" ],
"file": [ "list", "read" ],
"system": [ "board", "info" ],
"network.rrdns": [ "lookup" ]
Copy link
Contributor

Choose a reason for hiding this comment

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

There does not appear to be any reverse lookup calls in this code, so please drop this permission.

Comment on lines +14 to +17
"/www/luci-static/resources/view/status/include": [ "list" ],
"/etc/board.json": [ "read" ],
"/proc/sys/net/netfilter/nf_conntrack_count": [ "read" ],
"/proc/sys/net/netfilter/nf_conntrack_max": [ "read" ]
Copy link
Contributor

Choose a reason for hiding this comment

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

The simplified dashboard code does not appear to read any of those files, drop the read permissions for them.

},
"ubus": {
"luci": [ "getVersion" ],
"file": [ "list", "read" ],
Copy link
Contributor

Choose a reason for hiding this comment

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

There does not appear to be any filesystem listing calls, drop the list permission

"/proc/sys/net/netfilter/nf_conntrack_max": [ "read" ]
},
"ubus": {
"luci": [ "getVersion" ],
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears unused

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.

8 participants