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

Fixes Bunch #596

Merged
merged 3 commits into from
Jan 27, 2018
Merged

Fixes Bunch #596

merged 3 commits into from
Jan 27, 2018

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Jan 16, 2018

Related:

Goals

@hacdias hacdias changed the title Start using components from ipfs-react-components Reusable Components + New Files Pane Jan 16, 2018
@hacdias hacdias changed the title Reusable Components + New Files Pane Reusable Components Jan 16, 2018
@hacdias hacdias changed the title Reusable Components Reusable Components and some other fixes Jan 19, 2018
@hacdias hacdias self-assigned this Jan 20, 2018
@hacdias hacdias changed the title Reusable Components and some other fixes Fixes Bunch Jan 20, 2018
@hacdias hacdias changed the title Fixes Bunch [WIP] Fixes Bunch Jan 20, 2018
@hacdias
Copy link
Member Author

hacdias commented Jan 24, 2018

image

image

Most of the changes here were under the hood. It now uses MFS for the Files Pane, which does make a lot more sense.

Could you take a look @diasdavid?

@hacdias
Copy link
Member Author

hacdias commented Jan 24, 2018

Ping @olizilla too 😄

@daviddias
Copy link
Member

@hacdias are you storing the files to the mfs root folder or to a folder you create just for this app?

@hacdias
Copy link
Member Author

hacdias commented Jan 25, 2018

@diasdavid root folder. That way, the user can manage its files and won't be limited by Desktop's scope. Take a look at #590

@olizilla
Copy link
Member

@hacdias is fixing up an issue that showed up on my machine

10:35 AM <@hacdias> stats.repo and repo.stat do the same thing
10:35 AM <@hacdias> But on the api they dont have the same handler
10:35 AM <@hacdias> And I only changed one... I'll fix it!

@olizilla
Copy link
Member

There is a new issue preventing adding files via the files pane. @hacdias is investigating.

@olizilla
Copy link
Member

File adding now works! ✨

Buuuut I then got a little over excited and tired to add a large folder of pictures (around ~2GB). The UI got stuck immediatetly and after about 20s it stalled on:

screen shot 2018-01-25 at 11 24 39

It may well have been adding all the files in the background, but after a few minutes of waiting I gave up, only to have my entire system become unresponsive. Killing all the ipfs processes i could find didn't help, and I eventutually had to hard reboot! Apart from the UI stall, I don't think this is an issue with this PR, but I thought I should flag it as, I'm not going to be the only person who thinks ipfs might be a good way to share all the cat gifs.

What can we do to handle large file drops? Can we pull things off the ui thread so that it can warn the user about what's about to happen if they continue and show progress without freezing if they do?

Much more minor but also suprising was having a default electron start screen appear when restarting.

screen shot 2018-01-25 at 11 40 51

I assume this is just an artefact of running it in dev mode, but is there a way to make it not happen?

@olizilla
Copy link
Member

Deleting files seems a little ruthless right now. Did you have any thoughts on how we could make deleteing be un-doable? The button is right next to the copy hash button, so it should either request confirmation of a destructive action, or be easy to undo if they user clicks delete accidentally.

@hacdias
Copy link
Member Author

hacdias commented Jan 25, 2018

@olizilla :o

Buuuut I then got a little over excited and tired to add a large folder of pictures (around ~2GB). The UI got stuck immediatetly and after about 20s it stalled on:

Hmm... I have uploaded movies (~2GB) without any problem here.

Deleting files seems a little ruthless right now. Did you have any thoughts on how we could make deleteing be un-doable?

What if we just for confirmation telling that it is permanent?

I assume this is just an artefact of running it in dev mode, but is there a way to make it not happen?

I have never seen that before.

@olizilla
Copy link
Member

Channelling daviddias, master of the doodles on screenshots style feedback...

ipfs-desktop-files-pane

@hacdias
Copy link
Member Author

hacdias commented Jan 25, 2018

What do you do?

Yes, I'd like to have a better icon for that 😆 It was already suggested to use a pin but then it could be confused with the Pin tab.


I'll do the other updates later today since I won't be home during most of the evening.

@olizilla
Copy link
Member

What if we just for confirmation telling that it is permanent?

I think it's worth adding that. I find most confirmation boxes annoying ("what!? just do what i asked you to do computer!") but in the case where people could lose files by accident, it's worth it.

I'll do some more tests of the file upload issue.

@olizilla
Copy link
Member

Adding a single large file works well. Adding a directory tree with more 1 level seems to cause the super long add time...

@hacdias
Copy link
Member Author

hacdias commented Jan 26, 2018

image

image

@hacdias hacdias changed the title [WIP] Fixes Bunch Fixes Bunch Jan 26, 2018
@hacdias
Copy link
Member Author

hacdias commented Jan 26, 2018

@olizilla @lidel do you feel this branch can be safely merged into master?

@lidel
Copy link
Member

lidel commented Jan 26, 2018

I smoke-tested under Linux (Debian 9.3) and got this odd error but no logs:

2018-01-26-201024_1098x388_scrot

Note that last release (v0.3.0) works fine on the same machine.
Is there something I can do to provide more diagnostics?

@hacdias
Copy link
Member Author

hacdias commented Jan 26, 2018

set DEBUG='desktop' (or whatever is the linux way to set env vars) probably will give you some output. Did you remove node_modules first? There are some issues with some dependencies and you probably need to install them again.

@lidel
Copy link
Member

lidel commented Jan 26, 2018

Cleared node_modules + package-lock.json, run install and start with DEBUG flag:

lycia> DEBUG='desktop' npm start

> ipfs-desktop@0.3.0 start /home/lidel/projects/ipfs-desktop
> electron-forge start

✔ Checking your system
✔ Locating Application
✔ Preparing native dependencies: 2 / 3
✔ Launching Application
  desktop Application is ready +0ms
  desktop Starting daemon +6ms
  desktop Daemon started +3s
  desktop Uncaught Exception: TypeError: Cannot read property 'map' of null
  desktop     at transform (/home/lidel/projects/ipfs-desktop/node_modules/ipfs-api/src/files/ls.js:6:29)
  desktop     at send (/home/lidel/projects/ipfs-desktop/node_modules/ipfs-api/src/utils/send-request.js:208:7)
  desktop     at f (/home/lidel/projects/ipfs-desktop/node_modules/once/once.js:25:25)
  desktop     at streamToValue (/home/lidel/projects/ipfs-desktop/node_modules/ipfs-api/src/utils/stream-to-json-value.js:30:5)
  desktop     at concat (/home/lidel/projects/ipfs-desktop/node_modules/ipfs-api/src/utils/stream-to-value.js:12:22)
  desktop     at ConcatStream.<anonymous> (/home/lidel/projects/ipfs-desktop/node_modules/concat-stream/index.js:36:43)
  desktop     at emitNone (events.js:110:20)
  desktop     at ConcatStream.emit (events.js:207:7)
  desktop     at finishMaybe (/home/lidel/projects/ipfs-desktop/node_modules/readable-stream/lib/_stream_writable.js:607:14)
  desktop     at afterWrite (/home/lidel/projects/ipfs-desktop/node_modules/readable-stream/lib/_stream_writable.js:470:3) +40ms

Note that repeated test with yarn instead of npm (npx yarn@1.3.2 && DEBUG='desktop' npx yarn@1.3.2 start) and the result is the same.
I'll check the other box in a few mins.

@hacdias
Copy link
Member Author

hacdias commented Jan 26, 2018

@lidel sorry! I've submited a fix for that (ipfs-inactive/js-ipfs-http-client#680) but it's on js-ipfs-api! To "fix" that temporarily, add a file to the root of your MFS and I promise it will work 😄

@diasdavid could you take a look at that bugfix, please?

@lidel
Copy link
Member

lidel commented Jan 26, 2018

Cool, glad my smoke-testing is useful :) I created a file in MFS and it now starts, but .. oh boy, looks like CSS is missing?

2018-01-26-204215_615x420_scrot

If I scroll, i can find not-styled buttons:

2018-01-26-204228_619x420_scrot

Note that there are no errors in console nor in ~/.config/ipfs-desktop/logs/* 🤔
Did you see it before? DEBUG='desktop' shows nothing, so perhaps this one is caused by something on my end?

@hacdias
Copy link
Member Author

hacdias commented Jan 26, 2018

@lidel ugh that's weird... and ugly! Really... really... strange. Never happened. Try reloading with CTRL-R. You can open the dev tools with CTRL-SHIFT-I too

@hacdias
Copy link
Member Author

hacdias commented Jan 27, 2018

Any updates @lidel @olizilla ?

Edit @lidel try pulling the updates. I noticed something was wrong with the styles indeed.

@lidel
Copy link
Member

lidel commented Jan 27, 2018

@hacdias updates did not fix it, but I was able to find error related to app.less -- it references files which are not present(?), eg:

2018-01-27-195442_2158x472_scrot

lycia> grep Pane src/styles/app.less
@import "./components/Pane.less";
@import "./components/PaneContainer.less";

lycia> tree src/styles/ | grep Pane
│   ├── PaneContainer.less

I see that app.less includes other files that are not in git.
I think either one of implicit build steps fail on my machine or not everything is checked in.

@hacdias
Copy link
Member Author

hacdias commented Jan 27, 2018

@lidel, I'm sorry. Could you try now? (It was beginning with a lower case letter instead of an upper case).

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Good news: version from 28608cd solved style issues I had 👌

@hacdias
Copy link
Member Author

hacdias commented Jan 27, 2018

Reallllly nice @lidel 😄

@diasdavid @olizilla can we merge this?

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@8d7b93d). Click here to learn what that means.
The diff coverage is 8.75%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #596   +/-   ##
========================================
  Coverage          ?   4.29%           
========================================
  Files             ?      56           
  Lines             ?     839           
  Branches          ?       0           
========================================
  Hits              ?      36           
  Misses            ?     803           
  Partials          ?       0
Impacted Files Coverage Δ
src/components/PinnedHash.js 0% <ø> (ø)
src/panes/Settings.js 0% <ø> (ø)
src/components/Key.js 100% <ø> (ø)
src/screens/welcome.js 0% <ø> (ø)
src/components/MenuOption.js 0% <ø> (ø)
src/panes/Loader.js 0% <ø> (ø)
src/components/PaneContainer.js 0% <ø> (ø)
src/components/Header.js 0% <ø> (ø)
src/components/CheckboxBlock.js 0% <ø> (ø)
src/components/IconButton.js 0% <ø> (ø)
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d7b93d...d0a3c8b. Read the comment docs.

@hacdias
Copy link
Member Author

hacdias commented Jan 27, 2018

I'm merging this and if some other issue arises, we can fix it in another PR 😄 Let's move faster 😃

@hacdias hacdias merged commit 4fb4ccf into master Jan 27, 2018
@hacdias hacdias deleted the feat/components branch January 27, 2018 20:54
@makew0rld
Copy link

Great update, but it doesn't seem to fix #600 , at least not for me. It still doesn't show hashes I've pinned through the command line. 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants