Skip to content
This repository has been archived by the owner on Jun 16, 2018. It is now read-only.

Display the address of the server #258

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Display the address of the server #258

wants to merge 5 commits into from

Conversation

ThinkRedstone
Copy link

solves #242 .
Colour of the address is subject to change; To me, it seems the easiest to see.

I don't know if there's a better way to display the server IP without adding a new middleware and adding a new file.

address is not a very good name for that file, should probably be changed, but I have no idea for a better name.

@ThinkRedstone ThinkRedstone changed the title Display address Display the address of the server Aug 19, 2017
<div class="drawer" ng-class="{visible:drawerVisible}">
<div class="drawerLead" style="text-align: center; color: darkblue">
Copy link
Member

Choose a reason for hiding this comment

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

There shouldn't be constant styles in the HTML directly. Add a class and use CSS for the styling

var address = interfaces[i][j];//we basically parse over all the addresses for every network interface
if (address.family === 'IPv4' && !address.internal) {//if that address is an IPv4 address, and it is not an internal one, we add it
addresses.push(address.address);
}
Copy link
Member

Choose a reason for hiding this comment

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

You only use the first you find, but you continue to iterate over all of them. Use break or add the condition into the loop condition. Also you can keep only one in memory, but that's not really important

@poelstra
Copy link
Collaborator

Not sure about this one. Some thoughts:

  • Seems a bit strange that one has to first visit the webpage, to then see its address. But then one has already used the address...
  • Wouldn't it be better to print all possible addresses on the console of the server when starting it?
  • Wouldn't the Launcher that @2roy999 is creating be a better place for this type of thing?
  • When the server has multiple IP's (e.g. wireless and wired), it's confusing if it e.g. shows me the wired IP, when I connected to it's wireless IP, or vice-versa
  • Although technically simple, it's architecturally strange to save the server's IP address (well, one of it) in a user's session, because it has nothing to do with that user. A better place would be an API call (but again, that would be pretty silly, given that we already knew the IP to connect to in the first place...)

@@ -13,7 +13,8 @@ var middlewareLayers = [express.static(fileSystem.resolve('src')),
require('./server_modules/cors').middleware,
require('./server_modules/cache').middleware,
require('./server_modules/body_builder').middleware,
require('./server_modules/log').middleware];
require('./server_modules/log').middleware,
require('./server_modules/address').middleware];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better to place the closing bracket on its own line, to prevent unnecessary changes to the last line (e.g. how the log line is now changed).

var os = require('os');
var args = require('./args')

//shamelessly copied from StackOverflow with only minor changes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Although this is simple code, copying stuff without a clear license may conflict with the license of this project.

<div class="drawer" ng-class="{visible:drawerVisible}">
<div class="drawerLead">
address: {{serverAddress}}</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wrong indentation: the text should be indented, the closing tag on its own line.

@poelstra
Copy link
Collaborator

@alanggreen Can you give an update on your internal discussions about this?

@alanggreen
Copy link

@poelstra Discussed the idea of putting the address/port information in the tray with Roy. It's a possibility but not at this moment as he is focused on getting the installer up and running. That is my number 1 priority at the moment. If he can get that done then we can ask @ThinkRedstone to make this change.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants