Skip to content
This repository has been archived by the owner on Nov 14, 2022. It is now read-only.

Added session UI for tab close #68

Merged
merged 10 commits into from
Nov 7, 2019
Merged

Added session UI for tab close #68

merged 10 commits into from
Nov 7, 2019

Conversation

satanb4
Copy link
Contributor

@satanb4 satanb4 commented Nov 4, 2019

Description

This is a PR for fixing issues in #66 More changes will be pushed incrementally

Fixes issue #66

Motivation and Context

Tab closing and session confusion

#66 Session closing and log generation

How Has This Been Tested?

  • Ran our included tests
  • Manual QA
  • New test written

Screenshots (if appropriate):

Closed Tab

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@0xHericles 0xHericles self-requested a review November 4, 2019 10:37
@0xHericles
Copy link
Owner

@satanb4 The status does not refer to the tab but to the application's connection to the server. 🤔

@satanb4
Copy link
Contributor Author

satanb4 commented Nov 4, 2019

@satanb4 The status does not refer to the tab but to the application's connection to the server. 🤔

Then why have we been focusing on closing the tab's connection to the server? Also, from the user point of view of the tab session being closed, it makes more sense. Please go through issue #66 where I have discussed this.

@0xHericles 0xHericles mentioned this pull request Nov 4, 2019
2 tasks
.travis.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@vladdoster vladdoster left a comment

Choose a reason for hiding this comment

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

Could you add the plumbing to add Firefox to the Travis CI pipeline? It should be trivial.

tests/test_cast.py Outdated Show resolved Hide resolved
tests/test_cast.py Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
@satanb4
Copy link
Contributor Author

satanb4 commented Nov 5, 2019

Commit still buggy, when ready for review, I will request

@0xHericles
Copy link
Owner

Okay, @satanb4. I'll be waiting for you! 😉

@satanb4
Copy link
Contributor Author

satanb4 commented Nov 6, 2019

I expect to run into some bugs in this commit, but review it for this version (v0.0.1) nonetheless. This addresses issues #47 #66 and #42 . I have added checks for the bash exit command. As well as closed tabs that will now remain closed when backend session closes and reopens.

@satanb4
Copy link
Contributor Author

satanb4 commented Nov 6, 2019

Can be reviewed

Copy link
Owner

@0xHericles 0xHericles left a comment

Choose a reason for hiding this comment

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

@satanb4 Thanks for that. But can you let the status refer to the application's connection to the server again, please? We can talk about it after, but for now, we don't want to show the tab status. Now, if the tab was disconnected, it is closed, and vice-versa. So, the "tab status" it's more redundancy, and we lost a functionality: show the application status.

@satanb4
Copy link
Contributor Author

satanb4 commented Nov 6, 2019

I'll get that done

@satanb4
Copy link
Contributor Author

satanb4 commented Nov 7, 2019

I have fixed the tab connection. As well as performance improvement. Review requested

@satanb4 satanb4 requested a review from 0xHericles November 7, 2019 06:32
Copy link
Collaborator

@vladdoster vladdoster left a comment

Choose a reason for hiding this comment

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

LGTM.

@satanb4
Copy link
Contributor Author

satanb4 commented Nov 7, 2019

LGTM.

Need the coveralls repo token

Copy link
Owner

@0xHericles 0xHericles left a comment

Choose a reason for hiding this comment

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

GJ, @satanb4! 🤓

@0xHericles 0xHericles merged commit 931ad00 into 0xHericles:dev Nov 7, 2019
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.

3 participants