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

If bell() gets called emit bell event and add default bell sound #878

Merged
merged 24 commits into from
Aug 19, 2017

Conversation

npezza93
Copy link
Contributor

Fixes #853

Emits a bell event so you could hook into it and play a custom sound, or you can just use the default bell sound.

@coveralls
Copy link

coveralls commented Aug 10, 2017

Coverage Status

Coverage decreased (-0.06%) to 71.589% when pulling b3c16bf on npezza93:add-bell-sound into ea07bf8 on sourcelair:master.

@coveralls
Copy link

coveralls commented Aug 10, 2017

Coverage Status

Coverage decreased (-0.06%) to 71.589% when pulling 9c4fb2c on npezza93:add-bell-sound into ea07bf8 on sourcelair:master.

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

We're actually building from the branch v3 which has changed quite a bit. I can adapt this change for that branch if we go ahead with it though.

@@ -0,0 +1 @@
export const Bell = 'data:audio/ogg;base64,T2dnUwACAAAAAAAAAADhqW5KAAAAAMFvEjYBHgF2b3JiaXMAAAAAAYC7AAAAAAAAAHcBAAAAAAC4AU9nZ1MAAAAAAAAAAAAA4aluSgEAAAAAesI3EC3//////////////////8kDdm9yYmlzHQAAAFhpcGguT3JnIGxpYlZvcmJpcyBJIDIwMDkwNzA5AAAAAAEFdm9yYmlzKUJDVgEACAAAADFMIMWA0JBVAAAQAABgJCkOk2ZJKaWUoSh5mJRISSmllMUwiZiUicUYY4wxxhhjjDHGGGOMIDRkFQAABACAKAmOo+ZJas45ZxgnjnKgOWlOOKcgB4pR4DkJwvUmY26mtKZrbs4pJQgNWQUAAAIAQEghhRRSSCGFFGKIIYYYYoghhxxyyCGnnHIKKqigggoyyCCDTDLppJNOOumoo4466ii00EILLbTSSkwx1VZjrr0GXXxzzjnnnHPOOeecc84JQkNWAQAgAAAEQgYZZBBCCCGFFFKIKaaYcgoyyIDQkFUAACAAgAAAAABHkRRJsRTLsRzN0SRP8ixREzXRM0VTVE1VVVVVdV1XdmXXdnXXdn1ZmIVbuH1ZuIVb2IVd94VhGIZhGIZhGIZh+H3f933f930gNGQVACABAKAjOZbjKaIiGqLiOaIDhIasAgBkAAAEACAJkiIpkqNJpmZqrmmbtmirtm3LsizLsgyEhqwCAAABAAQAAAAAAKBpmqZpmqZpmqZpmqZpmqZpmqZpmmZZlmVZlmVZlmVZlmVZlmVZlmVZlmVZlmVZlmVZlmVZlmVZlmVZQGjIKgBAAgBAx3Ecx3EkRVIkx3IsBwgNWQUAyAAACABAUizFcjRHczTHczzHczxHdETJlEzN9EwPCA1ZBQAAAgAIAAAAAABAMRzFcRzJ0SRPUi3TcjVXcz3Xc03XdV1XVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVYHQkFUAAAQAACGdZpZqgAgzkGEgNGQVAIAAAAAYoQhDDAgNWQUAAAQAAIih5CCa0JrzzTkOmuWgqRSb08GJVJsnuamYm3POOeecbM4Z45xzzinKmcWgmdCac85JDJqloJnQmnPOeRKbB62p0ppzzhnnnA7GGWGcc85p0poHqdlYm3POWdCa5qi5FJtzzomUmye1uVSbc84555xzzjnnnHPOqV6czsE54Zxzzonam2u5CV2cc875ZJzuzQnhnHPOOeecc84555xzzglCQ1YBAEAAAARh2BjGnYIgfY4GYhQhpiGTHnSPDpOgMcgppB6NjkZKqYNQUhknpXSC0JBVAAAgAACEEFJIIYUUUkghhRRSSCGGGGKIIaeccgoqqKSSiirKKLPMMssss8wyy6zDzjrrsMMQQwwxtNJKLDXVVmONteaec645SGultdZaK6WUUkoppSA0ZBUAAAIAQCBkkEEGGYUUUkghhphyyimnoIIKCA1ZBQAAAgAIAAAA8CTPER3RER3RER3RER3RER3P8RxREiVREiXRMi1TMz1VVFVXdm1Zl3Xbt4Vd2HXf133f141fF4ZlWZZlWZZlWZZlWZZlWZZlCUJDVgEAIAAAAEIIIYQUUkghhZRijDHHnINOQgmB0JBVAAAgAIAAAAAAR3EUx5EcyZEkS7IkTdIszfI0T/M00RNFUTRNUxVd0RV10xZlUzZd0zVl01Vl1XZl2bZlW7d9WbZ93/d93/d93/d93/d939d1IDRkFQAgAQCgIzmSIimSIjmO40iSBISGrAIAZAAABACgKI7iOI4jSZIkWZImeZZniZqpmZ7pqaIKhIasAgAAAQAEAAAAAACgaIqnmIqniIrniI4oiZZpiZqquaJsyq7ruq7ruq7ruq7ruq7ruq7ruq7ruq7ruq7ruq7ruq7ruq7rukBoyCoAQAIAQEdyJEdyJEVSJEVyJAcIDVkFAMgAAAgAwDEcQ1Ikx7IsTfM0T/M00RM90TM9VXRFFwgNWQUAAAIACAAAAAAAwJAMS7EczdEkUVIt1VI11VItVVQ9VVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVV1TRN0zSB0JCVAAAZAAAjQQYZhBCKcpBCbj1YCDHmJAWhOQahxBiEpxAzDDkNInSQQSc9uJI5wwzz4FIoFURMg40lN44gDcKmXEnlOAhCQ1YEAFEAAIAxyDHEGHLOScmgRM4xCZ2UyDknpZPSSSktlhgzKSWmEmPjnKPSScmklBhLip2kEmOJrQAAgAAHAIAAC6HQkBUBQBQAAGIMUgophZRSzinmkFLKMeUcUko5p5xTzjkIHYTKMQadgxAppRxTzinHHITMQeWcg9BBKAAAIMABACDAQig0ZEUAECcA4HAkz5M0SxQlSxNFzxRl1xNN15U0zTQ1UVRVyxNV1VRV2xZNVbYlTRNNTfRUVRNFVRVV05ZNVbVtzzRl2VRV3RZV1bZl2xZ+V5Z13zNNWRZV1dZNVbV115Z9X9ZtXZg0zTQ1UVRVTRRV1VRV2zZV17Y1UXRVUVVlWVRVWXZlWfdVV9Z9SxRV1VNN2RVVVbZV2fVtVZZ94XRVXVdl2fdVWRZ+W9eF4fZ94RhV1dZN19V1VZZ9YdZlYbd13yhpmmlqoqiqmiiqqqmqtm2qrq1bouiqoqrKsmeqrqzKsq+rrmzrmiiqrqiqsiyqqiyrsqz7qizrtqiquq3KsrCbrqvrtu8LwyzrunCqrq6rsuz7qizruq3rxnHrujB8pinLpqvquqm6um7runHMtm0co6rqvirLwrDKsu/rui+0dSFRVXXdlF3jV2VZ921fd55b94WybTu/rfvKceu60vg5z28cubZtHLNuG7+t+8bzKz9hOI6lZ5q2baqqrZuqq+uybivDrOtCUVV9XZVl3zddWRdu3zeOW9eNoqrquirLvrDKsjHcxm8cuzAcXds2jlvXnbKtC31jyPcJz2vbxnH7OuP2daOvDAnHjwAAgAEHAIAAE8pAoSErAoA4AQAGIecUUxAqxSB0EFLqIKRUMQYhc05KxRyUUEpqIZTUKsYgVI5JyJyTEkpoKZTSUgehpVBKa6GU1lJrsabUYu0gpBZKaS2U0lpqqcbUWowRYxAy56RkzkkJpbQWSmktc05K56CkDkJKpaQUS0otVsxJyaCj0kFIqaQSU0mptVBKa6WkFktKMbYUW24x1hxKaS2kEltJKcYUU20txpojxiBkzknJnJMSSmktlNJa5ZiUDkJKmYOSSkqtlZJSzJyT0kFIqYOOSkkptpJKTKGU1kpKsYVSWmwx1pxSbDWU0lpJKcaSSmwtxlpbTLV1EFoLpbQWSmmttVZraq3GUEprJaUYS0qxtRZrbjHmGkppraQSW0mpxRZbji3GmlNrNabWam4x5hpbbT3WmnNKrdbUUo0txppjbb3VmnvvIKQWSmktlNJiai3G1mKtoZTWSiqxlZJabDHm2lqMOZTSYkmpxZJSjC3GmltsuaaWamwx5ppSi7Xm2nNsNfbUWqwtxppTS7XWWnOPufVWAADAgAMAQIAJZaDQkJUAQBQAAEGIUs5JaRByzDkqCULMOSepckxCKSlVzEEIJbXOOSkpxdY5CCWlFksqLcVWaykptRZrLQAAoMABACDABk2JxQEKDVkJAEQBACDGIMQYhAYZpRiD0BikFGMQIqUYc05KpRRjzknJGHMOQioZY85BKCmEUEoqKYUQSkklpQIAAAocAAACbNCUWByg0JAVAUAUAABgDGIMMYYgdFQyKhGETEonqYEQWgutddZSa6XFzFpqrbTYQAithdYySyXG1FpmrcSYWisAAOzAAQDswEIoNGQlAJAHAEAYoxRjzjlnEGLMOegcNAgx5hyEDirGnIMOQggVY85BCCGEzDkIIYQQQuYchBBCCKGDEEIIpZTSQQghhFJK6SCEEEIppXQQQgihlFIKAAAqcAAACLBRZHOCkaBCQ1YCAHkAAIAxSjkHoZRGKcYglJJSoxRjEEpJqXIMQikpxVY5B6GUlFrsIJTSWmw1dhBKaS3GWkNKrcVYa64hpdZirDXX1FqMteaaa0otxlprzbkAANwFBwCwAxtFNicYCSo0ZCUAkAcAgCCkFGOMMYYUYoox55xDCCnFmHPOKaYYc84555RijDnnnHOMMeecc845xphzzjnnHHPOOeecc44555xzzjnnnHPOOeecc84555xzzgkAACpwAAAIsFFkc4KRoEJDVgIAqQAAABFWYowxxhgbCDHGGGOMMUYSYowxxhhjbDHGGGOMMcaYYowxxhhjjDHGGGOMMcYYY4wxxhhjjDHGGGOMMcYYY4wxxhhjjDHGGGOMMcYYY4wxxhhjjDHGGFtrrbXWWmuttdZaa6211lprrQBAvwoHAP8HG1ZHOCkaCyw0ZCUAEA4AABjDmHOOOQYdhIYp6KSEDkIIoUNKOSglhFBKKSlzTkpKpaSUWkqZc1JSKiWlllLqIKTUWkottdZaByWl1lJqrbXWOgiltNRaa6212EFIKaXWWostxlBKSq212GKMNYZSUmqtxdhirDGk0lJsLcYYY6yhlNZaazHGGGstKbXWYoy1xlprSam11mKLNdZaCwDgbnAAgEiwcYaVpLPC0eBCQ1YCACEBAARCjDnnnHMQQgghUoox56CDEEIIIURKMeYcdBBCCCGEjDHnoIMQQgghhJAx5hx0EEIIIYQQOucchBBCCKGEUkrnHHQQQgghlFBC6SCEEEIIoYRSSikdhBBCKKGEUkopJYQQQgmllFJKKaWEEEIIoYQSSimllBBCCKWUUkoppZQSQgghlFJKKaWUUkIIoZRQSimllFJKCCGEUkoppZRSSgkhhFBKKaWUUkopIYQSSimllFJKKaUAAIADBwCAACPoJKPKImw04cIDUGjISgCADAAAcdhq6ynWyCDFnISWS4SQchBiLhFSijlHsWVIGcUY1ZQxpRRTUmvonGKMUU+dY0oxw6yUVkookYLScqy1dswBAAAgCAAwECEzgUABFBjIAIADhAQpAKCwwNAxXAQE5BIyCgwKx4Rz0mkDABCEyAyRiFgMEhOqgaJiOgBYXGDIB4AMjY20iwvoMsAFXdx1IIQgBCGIxQEUkICDE2544g1PuMEJOkWlDgIAAAAA4AAAHgAAkg0gIiKaOY4Ojw+QEJERkhKTE5QAAAAAALABgA8AgCQFiIiIZo6jw+MDJERkhKTE5AQlAAAAAAAAAAAACAgIAAAAAAAEAAAACAhPZ2dTAAQYOwAAAAAAAOGpbkoCAAAAmc74DRgyNjM69TAzOTk74dnLubewsbagmZiNp4d0KbsExSY/I3XUTwJgkeZdn1HY4zoj33/q9DFtv3Ui1/jmx7lCUtPt18/sYf9MkgAsAGRBd3gMGP4sU+qCPYBy9VrA3YqJosW3W2/ef1iO/u3cg8ZG/57jU+pPmbGEJUgkfnaI39DbPqxddZphbMRmCc5rKlkUMkyx8iIoug5dJv1OYH9a59c+3Gevqc7Z2XFdDjL/qHztRfjWEWxJ/aiGezjohu9HsCZdQBKbiH0VtU/3m85lDG2T/+xkZcYnX+E+aqzv/xTgOoTFG+x7SNqQ4N+oAABSxuVXw77Jd5bmmTmuJakX7509HH0kGYKvARPpwfOSAPySPAc2EkneDwB2HwAAJlQDYK5586N79GJCjx4+p6aDUd27XSvRyXLJkIC5YZ1jLv5lpOhZTz0s+DmnF1diptrnM6UDgIW11Xh8cHTd0/SmbgOAdxcyWwMAAGIrZ3fNSfZbzKiYrK4+tPqtnMVLOeWOG2kVvUY+p2PJ/hkCl5aFRO4TLGYPZcIU3vYM1hohS4jHFlnyW/2T5J7kGsShXWT8N05V+3C/GPqJ1QdWisGPxEzHqXISBPIinWDUt7IeJv/f5OtzBxpTzZZQ+CYEhHXfqG4aABQli72GJhN4oJv+hXcApAJSErAW8G2raAX4NUcABnVt77CzZAB+LsHcVe+Q4h+QB1wh/ZrJTPxSBdI8mgTeAdTsQOoFUEng9BHcVPhxSRRYkKWZJXOFYP6V4AEripJoEjXgA2wJRZHSExmJDm8F0A6gEXsg5a4ZsALItrMB7+fh7UKLvYWSdtsDwFf1mzYzS1F82N1h2Oyt2e76B1QdS0SAsQigLPMOgJS9JRC7hFXA6kUsLFNKD5cA5cTRvgSqPc3Fl99xW3QTi/MHR8DEm6WnvaVQATwRqRKjywQ9BrrhugR2AKTsPQeQckrAOgDOhbTESyrXQ50CkNpXdtWjW7W2/3UjeX3U95gIdalfRAoAmqUEiwp53hCdcCwlg47fcbfzlmQMAgaBkh7c+fcDgF+ifwDXfzegLPcLYJsAAJQArTXjnh/uXGy3v1Hk3pV6/3t5ruW81f6prfbM2Q3WNVy98BwUtbCwhFhAWuPev6Oe/4ZaFQUcgKrVs4defzh1TADA1DEh5b3VlDaECw5b+bPfkKos3tIAue3vJZOih3ga3l6O3PSfIkrLv0PAS86PPdL7g8oc2KteNFKKzKRehOv2gJoFLBPXmaXvPBQILgJon0bbWBszrYZYYwE7jl2j+vTdU7Vpk21LiU0QajPkywAAHqbUC0/YsYOdb4e6BOp7E0cCi04Ao/TgD8ZVAMid6h/A8IeBNkp6/xsAACZELEYIk+yvI6Qz1NN6lIftB/6IMWjWJNOqPTMedAmyaj6Es0QBklJpiSWWHnQ2CoYbGWAmt+0gLQBFKCBnp2QUUQZ/1thtZDBJUpFWY82z34ocorB62oX7qB5y0oPAv/foxH25wVmgIHf2xFOr8leZcBq1Kx3ZvCq9Bga639AxuHuPNL/71YCF4EywJpqHFAX6XF0sjVbuANnvvdLcrufYwOM/iDa6iA468AYAAB6mNBMXcgTD8HSRqJ4vw8CjAlCEPACASlX/APwPOJKl9xQAAAPmnev2eWp33Xgyw3Dvfz6myGk3oyP8YTKsCOvzAgALQi0o1c6Nzs2O2Pg2h4ACIJAgAGP0aNn5x0BDgVfH7u2TtyfDcRIuYAyQhBF/lvSRAttgA6TPbWZA9gaUrZWAUEAA+Dx47Q3/r87HxUUqZmB0BmUuMlojFjHt1gDunnvuX8MImsjSq5WkzSzGS62OEIlOufWWezxWpv6FBgDgJVltfXFYtNAAnqU0xQoD0YLiXo5cF5QV4CnY1tBLAkZCOABAhbk/AM+/AwSCCdlWAAAMcFjS7owb8GVDzveDiZvznbt2tF4bL5odN1YKl88TAEABCZvufq9YCTBtMwVAQUEAwGtNltzSaHvADYC3TxLVjqiRA+OZAMhzcqEgRcAOwoCgvdTxsTHLQEF6+oOb2+PAI8ciPQcXg7pOY+LjxQSv2fjmFuj34gGwz310/bGK6z3xgT887eomWULEaDd04wHetYxdjcgV2SxvSwn0VoZXJRqkRC5ASQ/muVoAUsX7AgAQMBNaVwAAlABRxT/1PmfqLqSRNDbhXb07berpB3b94jpuWEZjBCD2OcdXFpCKEgCDfcFPMw8AAADUwT4lnUm50lmwrpMMhPQIKj6u0E8fr2vGBngMNdIlrZsigjahljud6AFVg+tzXwUnXL3TJLpajaWKA4VAAAAMiFfqJgKAZ08XrtS3dxtQNYcpPvYEG8ClvrQRJgBephwnNWJjtGqmp6VEPSvBe7EBiU3qgJbQAwD4Le8LAMDMhHbNAAAlgK+tFs5O+YyJc9yCnJa3rxLPulGnxwsXV9Fsk2k4PisCAHC8FkwbGE9gJQAAoMnyksj0CdFMZLLgoz8M+FxziwYBgIx+zHiCBAKAlBKNpF1sO9JpVcyEi9ar15YlHgrut5fPJnkdJ6vEwZPyAHQBIEDUrlMcBAAd2KAS0Qq+JwRsE4AJZtMnAD6GnOYwYlOIZvtzUNdjreB7fiMkWI0CmBB6AIAKc38A9osEFlTSGECB+cbeRDC0aRpLHqNPplcK/76Lxn2rpmqyXsYJWRi/FQAAAKBQk9MCAOibrQBQADCDsqpooPutd+05Ce9g6iEdiYXgVmQAI4+4wskEBEiBloNQ6Ki0/KTQ0QjWfjxzi+AeuXKoMjEVfQOZzr0y941qLgM2AExvbZOqcxZ6J6krlrj4y2j9AdgKDx6GnJsVLhbc42uq584+ouSdNBpoCiCVHrz+WzUA/DDtD8ATgA3h0lMCAAzcFv+S+fSSNkeYWlTpb34mf2RfmqqJeMeklhHAfu7VoAEACgAApKRktL+KkQDWMwYCUAAAAHCKsp80xhp91UjqQBw3x45cetqkjQEyu3G9B6N+R650Uq8OVig7wOm6Wun0ea4lKDPoabJs6aLqgbhPzpv4KR4iODilw88ZpY7q1IOMcbASAOAVtmcCnobcrkG4KGS7/ZnskVWRNF9J0RUHKOnByy9WA8Dv6L4AAARMCQUA4GritfVM2lcZfH3Q3T/vZ47J2YHhcmBazjfdyuV25gLAzrc0cwAAAAAYCh6PdwAAAGyWjFW4yScjaWa2mGcofHxWxewKALglWBpLUvwwk+UOh5eNGyUOs1/EF+pZr+ud5OzoGwYdAABg2p52LiSgAY/ZVlOmilEgHn6G3OcwYjzI7vOj1t6xsx4S3lBY96EUQBF6AIBAmPYH4PoGYCoJAADWe+OZJZi7/x76/yH7Lzf9M5XzRKnFPmveMsilQHwVAAAAAKB3LQD8PCIAAADga0QujBLywzeJ4a6Z/ERVBAUlAEDqvoM7BQBAuAguzFqILtmjH3Kd4wfKobnOhA3z85qWoRPm9hwoOHoDAAlCbwDAA56FHAuXflHo3fe2ttG9XUDeA9YmYCBQ0oPr/1QC8IvuCwAAApbUAQCK22MmE3O78VAbHQT9PIPNoT9zNc3l2Oe7TAVLANBufT8MAQAAAGzT4PS8AQAAoELGHb2uaCwwEv1EWhFriUkbAaAZ27/fVZnTZXbWz3BwWpjUaMZKRj7dZ0J//gUeTdpVEwAAZOFsNxKAjQSgA+ABPoY8Jj5y2wje81jsXc/1TOQWTDYZBmAkNDiqVwuA2NJ9AQAAEBKAt9Vrsfs/2N19MO91S9rd8EHTZHnzC5MYmfQEACy/FBcAAADA5c4gi4z8RANs/m6FNXVo9DV46JG1BBDukqlw/Va5G7QbuGVSI+2aZaoLXJrdVj2zlC9Z5QEAEFz/5QzgVZwAAAAA/oXcxyC6WfTu+09Ve/c766J4VTAGUFmA51+VANKi/QPoPwYgYAkA715OH4S0s5KDHvj99MMq8TPFc3roKZnGOoT1bmIhVgc7XAMBAAAAAMAW1VbQw3gapzOpJd+Kd2fc4iSO62fJv9+movui1wUNPAj059N3OVxzk4gV73PmE8FIA2F5mRq37Evc76vLXfF4rD5UJJAw46hW6LZCb5sNLdx+kzMCAAB+hfy95+965ZCLP7B3/VlTHCvDEKtQhTm4KiCgAEAbrfbWTPssAAAAXpee1tVrozYYn41wD1aeYtkKfswN5/SXPO0JDnhO/4laUortv/s412fybe/nONdncoCHnBVliu0CQGBWlPY/5Kwom2L/kruPM6Q7oz4tvDQy+bZ3HzOi+gNHA4DZEgA=';
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we can just pull in Chromium's bell as we would need to follow the license. Ideally a public domain sound would be used.

@parisk thoughts?

src/xterm.js Outdated
@@ -340,6 +341,7 @@ Terminal.defaults = {
cursorStyle: 'block',
visualBell: false,
popOnBell: false,
playDefaultBell: true,
Copy link
Member

Choose a reason for hiding this comment

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

Are these various settings meant to be mutually exclusive? Does it make sense for bellStyle to be an enum?

A URL could also be used which points at the sound to play so it's easily customizable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I refactored it a bit so now you can pass your own sound, false and no sound will be played, or leave it as it is and the default beep will be played. Thoughts?

src/xterm.js Outdated
@@ -28,6 +28,7 @@ import * as Mouse from './utils/Mouse';
import { CHARSETS } from './Charsets';
import { getRawByteCoords } from './utils/Mouse';
import { translateBufferLineToString } from './utils/BufferLine';
import { Bell } from './utils/Bell';
Copy link
Member

@Tyriar Tyriar Aug 10, 2017

Choose a reason for hiding this comment

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

Ideally import { BellSound } from './utils/Sounds';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohhh good idea!

src/xterm.js Outdated
@@ -1863,6 +1865,10 @@ Terminal.prototype.send = function(data) {
* Note: We could do sweet things with webaudio here
*/
Terminal.prototype.bell = function() {
this.emit('bell');
if (this.playDefaultBell) {
(new Audio(Bell)).play();
Copy link
Member

Choose a reason for hiding this comment

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

We should preload this like hterm, otherwise the first bell will be delayed.

@parisk
Copy link
Contributor

parisk commented Aug 10, 2017

Thanks for this PR @npezza93! Can you please change the base of your PR to the v3 branch and adjust anything that might need change?

@Tyriar
Copy link
Member

Tyriar commented Aug 10, 2017

@parisk my comment got hidden above, but AFAIK the BellSound needs us to declare Chromium's license. We can either do that or find a public domain one.

@Tyriar Tyriar changed the base branch from master to v3 August 10, 2017 17:22
@Tyriar
Copy link
Member

Tyriar commented Aug 10, 2017

I changed the base, I'll fix up the conflicts

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

I just brought it up to date and noticed a few things:

  • Terminal.bell was never called, it was being reimplemented in InputHandler. (fixed)
  • bellAudioElement is never added to the DOM so I'm not sure the audio gets preloaded. (not fixed)
  • bellAudioElement was being constructed on Terminal creation, it should be moved to Terminal.open as we don't assume there's a DOM until then. (not fixed)

@Tyriar
Copy link
Member

Tyriar commented Aug 10, 2017

I don't think popOnBell does anything really, how about this for the options format?

enum BellStyle {
  None,
  Sound,
  Visual
}

...

{
bellSound: string,
bellStyle: BellStyle
}

@mofux
Copy link
Contributor

mofux commented Aug 10, 2017

@Tyriar What if you want visual and acoustic feedback?

@Tyriar
Copy link
Member

Tyriar commented Aug 10, 2017

@mofux is that a likely scenario? BellStyle could be a flags enum if we really wanted to support that:

new Terminal({
  bellStyle: BellStyle.Sound | BellStyle.Visual
});

Keeping the number of options down is a good imo.

@mofux
Copy link
Contributor

mofux commented Aug 10, 2017

@Tyriar Yes, for example in hyper the block cursor flashes for a short moment and in parallel the bell sound rings. Btw. what would be the visual representation of the bell anyway? I really like the way hyper does it because it is subtle but still very effective because the eyes most likely focus around the cursor anyway.

@Tyriar
Copy link
Member

Tyriar commented Aug 10, 2017

Visual right now is a white border, not sure if it even works though. Hasn't been touched since this was forked from term.js

@npezza93
Copy link
Contributor Author

@mofux @Tyriar I like the way Hyper does the visual representation too and I think that would be cool to include in here. But I also agree with @Tyriar about keeping the amount of options limited and could argue that this can be done by the user by listening to the bell event. I'm game to implement either option. Thoughts?

@Tyriar Thanks for rebasing this for me! I'll try to get those issues sorted out this evening.

@mofux
Copy link
Contributor

mofux commented Aug 11, 2017

@npezza93 @Tyriar Animating the cursor is not easy, because the cursor element is replaced every time the screen content is redrawn. Maybe we should move the visual bell to a plugin? Actually, the best solution would be to always re-use the same element for the cursor, so we could animate on it...

src/Terminal.ts Outdated
if (this.options.bellStyles.indexOf(BellStyles.Visual) > -1) {
var cursor = this.element.querySelector('.terminal-cursor') as HTMLElement;
cursor.style.backgroundColor = "#fff";
setTimeout(function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should cancel a previous setTimeout when "bell" is called subsequently, otherwise an already running setTimeout would reset the cursor style while a subsequent timeout is still running.

src/Terminal.ts Outdated
@@ -1893,15 +1893,16 @@ export class Terminal extends EventEmitter implements ITerminal, IInputHandlingT
*/
public bell(): void {
this.emit('bell');
if (this.options.bellSound) {
if (this.options.bellStyles.indexOf(BellStyles.Sound) > -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be safe to use this.options.bellStyles.includes(BellStyles.Sound) which reads nicer

Copy link
Member

Choose a reason for hiding this comment

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

I don't think TypeScript will let you do this

Copy link
Contributor

Choose a reason for hiding this comment

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

@Tyriar You are right, sorry, includes on an array does not seem to be supported by TypeScript yet.

src/Terminal.ts Outdated
if (this.options.popOnBell) this.focus();
if (this.options.bellStyles.indexOf(BellStyles.Visual) > -1) {
var cursor = this.element.querySelector('.terminal-cursor') as HTMLElement;
cursor.style.backgroundColor = "#fff";
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of setting a background color directly, we should set a class on on the terminal element (e.g. visual-bell-active). Then we can style it via css .terminal.visual-bell-active .cursor { background-color: white }.

src/xterm.css Outdated
@@ -94,20 +94,20 @@
}

.terminal:not(.focus) .terminal-cursor {
outline: 1px solid #fff;
outline: 1px solid #e6e6e6;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep pure plain white as the default cursor color 😀

@npezza93
Copy link
Contributor Author

@Tyriar @mofux This is ready for another review. Looks like there's a buffer test failure but I don't think it was caused by my changes...Let me know what you guys think!

@mofux
Copy link
Contributor

mofux commented Aug 14, 2017

@npezza93 I'm not quite happy with the css yet. We have to make sure the visual bell works for all cursor types, at the moment it only looks okay for the block cursor. I'd also like to see a little transition effect when the background color of the cursor changes, which makes it feel more natural.

Here is some css I applied locally to make all the cursor styles work, and to transition the background color:

.terminal.focus.xterm-cursor-style-block.visual-bell-active .terminal-cursor {
  ... replaced your css selector to make it more specific ...
  background-color: #CCC;
}

.terminal.focus.xterm-cursor-style-bar.visual-bell-active .terminal-cursor::before,
.terminal.focus.xterm-cursor-style-underline.visual-bell-active .terminal-cursor::before {
  background-color: transparent;
}

.terminal.focus.xterm-cursor-style-bar:not(.xterm-cursor-blink-on) .terminal-cursor::before,
.terminal.focus.xterm-cursor-style-underline:not(.xterm-cursor-blink-on) .terminal-cursor::before {
    ... other rules that already exist ...
    transition: background-color 150ms ease;
}

.terminal .terminal-cursor {
    ... other rules that already exist ...
    transition: background-color 150ms ease;
}

Here's a demo how it looks like:
kapture 2017-08-14 at 23 28 40

@npezza93
Copy link
Contributor Author

Ahh good point @mofux. Thanks for pointing that out, I have updated the styling.

@@ -0,0 +1,5 @@
export enum BellStyles {
Copy link
Member

Choose a reason for hiding this comment

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

I think using flags works better here, also use the singular Style:

export enum BellStyle {
  None = 0,
  Sound = 1,
  Visual = 2
}

...

// Binary or for multiple:
bellStyle: BellStyle.Sound | BellStyle.Visual

Copy link
Member

Choose a reason for hiding this comment

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

Also let's move this into src/Types.ts directly since enums are types.

src/Terminal.ts Outdated
@@ -695,6 +699,14 @@ export class Terminal extends EventEmitter implements ITerminal, IInputHandlingT
this.viewportScrollArea.classList.add('xterm-scroll-area');
this.viewportElement.appendChild(this.viewportScrollArea);

// preload audio
if (this.options.bellStyles.indexOf(BellStyles.Sound) > -1) {
Copy link
Member

Choose a reason for hiding this comment

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

Currently this is add only when BellStyles.Sound is set on open. It also needs to be hooked up to the setOption function for when it's changed after it's already been opened.

src/Terminal.ts Outdated
visualBell: false,
popOnBell: false,
bellSound: BellSound,
bellStyles: [BellStyles.Sound, BellStyles.Visual],
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this singular: bellStyle, I also think the default should be none.

demo/main.js Outdated
@@ -70,7 +70,8 @@ function createTerminal() {
term = new Terminal({
cursorBlink: optionElements.cursorBlink.checked,
scrollback: parseInt(optionElements.scrollback.value, 10),
tabStopWidth: parseInt(optionElements.tabstopwidth.value, 10)
tabStopWidth: parseInt(optionElements.tabstopwidth.value, 10),
bellStyles: [1, 2]
Copy link
Member

Choose a reason for hiding this comment

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

We need some way of exporting BellStyle here so we don't just pass in numbers. Maybe export it on Terminal as well as exporting from the Terminal.ts file itself?

What I mean is supporting this for web:

new Terminal({
  bellStyle: Terminal.BellStyle.Sound
});

And this for Electron:

import { Terminal, BellStyle } from 'xterm';

new Terminal({
  bellStyle: BellStyle.Sound
});

Thoughts @parisk?

Copy link
Contributor

Choose a reason for hiding this comment

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

What about just using strings? For example:

// No bell
new Terminal({
  bellStyle: null
});

// Visual
new Terminal({
  bellStyle: 'visual'
});

// Audio
new Terminal({
  bellStyle: 'audio'
});

// Visual and Audio
new Terminal({
  bellStyle: 'both' // or ['audio', 'visual']
});

Copy link
Member

Choose a reason for hiding this comment

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

That's another option, I believe we can extend the StringOption type checking later to include values as well which is probably the best of both worlds (standard JS practice, strong typings in TS).

https://github.com/sourcelair/xterm.js/blob/39328d6e0c05b1f1f574303ad0288687ca6f813f/src/Types.ts#L33-L35

We can follow that up in #889

@parisk
Copy link
Contributor

parisk commented Aug 16, 2017

In response to #878 (comment): I would prefer if we went with a public domain one (example).

@npezza93
Copy link
Contributor Author

@Tyriar and @parisk, I have made the requested changes, let me know what you think! Also, want me to rebase to get rid of the conflicts?

@mofux
Copy link
Contributor

mofux commented Aug 16, 2017

@npezza93 👍 rebase please 😅 That should make looking through the changes a lot easier.

@npezza93 npezza93 force-pushed the add-bell-sound branch 3 times, most recently from d54587a to 5088dc3 Compare August 17, 2017 00:38
Allow the bellSound to be more easily configurable. You can pass false
to the bellSound and no sound will be played or you can pass it any
source you'd like.
@mofux
Copy link
Contributor

mofux commented Aug 18, 2017

Like it, but it's quite long imo.
1.4 seconds vs 0.2 for https://freesound.org/people/joseph.larralde/sounds/352501/ for example.

Try to search for key - that also gives nice short results:
https://freesound.org/search/?q=key&f=duration%3A%5B0+TO+1%5D&s=score+desc&advanced=1&g=1

@mofux
Copy link
Contributor

mofux commented Aug 18, 2017

I've just fallen in love with this one: https://freesound.org/people/jonnay/sounds/3168/ and its muted version https://freesound.org/people/altemark/sounds/45759/.

Look at the wave! And it's only 0.001s long!

@npezza93
Copy link
Contributor Author

npezza93 commented Aug 18, 2017

What if I could trim the audio? 😄
(The audio is in the zip)

bell.zip

@npezza93
Copy link
Contributor Author

@mofux Oh I like that one!

@mofux
Copy link
Contributor

mofux commented Aug 18, 2017

@npezza93 Your short bell version is also very good!

Time to spread some emojis guys! Choose the reaction of the sound you like most, if none of them wins we'll open up another round:
👍 https://github.com/sourcelair/xterm.js/files/1232998/bell.zip
❤️ https://freesound.org/people/jonnay/sounds/3168/
🎉 https://freesound.org/people/altemark/sounds/45759/
👎 None of them

@parisk
Copy link
Contributor

parisk commented Aug 18, 2017

I voted ❤️, as it's a little bit more discreet.

@parisk
Copy link
Contributor

parisk commented Aug 19, 2017

Should we go with "🎉 https://freesound.org/people/altemark/sounds/45759/" then 😄?

@coveralls
Copy link

coveralls commented Aug 19, 2017

Coverage Status

Coverage decreased (-0.09%) to 71.795% when pulling b0164e1 on npezza93:add-bell-sound into f3d375a on sourcelair:v3.

@Tyriar
Copy link
Member

Tyriar commented Aug 19, 2017

It's nice and small too, might not be worth bothering with lazy loading if it's that small

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 71.795% when pulling 668b25e on npezza93:add-bell-sound into f3d375a on sourcelair:v3.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 71.795% when pulling 668b25e on npezza93:add-bell-sound into f3d375a on sourcelair:v3.

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

FYI I fixed up licensing stuff around the sound as it's not public domain.

LGTM after the final changes below

src/xterm.css Outdated
@@ -124,6 +126,14 @@
height: 1px;
}

.terminal.focus.xterm-cursor-style-block.visual-bell-active .terminal-cursor {
background-color: #ccc !important;
Copy link
Member

Choose a reason for hiding this comment

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

Might be better to transition on opacity so that consumers that use custom themes don't need to deal with the visual bell?

src/Terminal.ts Outdated
this.bellAudioElement.setAttribute('src', this.options.bellSound);
this.element.appendChild(this.bellAudioElement);
} else if (this.bellAudioElement) {
this.bellAudioElement.remove();
Copy link
Member

Choose a reason for hiding this comment

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

Support for ChildNode.remove isn't very good, we should use Node.removeChild instead to improve compatibility.

@npezza93
Copy link
Contributor Author

Should be good now @Tyriar!

@coveralls
Copy link

coveralls commented Aug 19, 2017

Coverage Status

Coverage decreased (-0.09%) to 71.795% when pulling 1211435 on npezza93:add-bell-sound into f3d375a on sourcelair:v3.

src/xterm.css Outdated
}
.terminal.focus.xterm-cursor-style-bar.visual-bell-active .terminal-cursor::before,
.terminal.focus.xterm-cursor-style-underline.visual-bell-active .terminal-cursor::before {
background-color: transparent;
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs to be opacity: 0

src/xterm.css Outdated
@@ -91,6 +91,7 @@

.terminal .terminal-cursor {
position: relative;
transition: background-color 150ms ease;
Copy link
Member

Choose a reason for hiding this comment

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

transition: opacity...?

src/xterm.css Outdated
@@ -108,6 +109,7 @@
content: '';
position: absolute;
background-color: #fff;
transition: background-color 150ms ease;
Copy link
Member

Choose a reason for hiding this comment

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

transition: opacity...?

src/Terminal.ts Outdated
this.bellAudioElement.setAttribute('src', this.options.bellSound);
this.element.appendChild(this.bellAudioElement);
} else if (this.bellAudioElement) {
this.element.removeChild(this.bellAudioElement);
Copy link
Member

Choose a reason for hiding this comment

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

Now that I'm reading this again, it should be added/removed to this.helperContainer instead of this.element.

@coveralls
Copy link

coveralls commented Aug 19, 2017

Coverage Status

Coverage decreased (-0.09%) to 71.795% when pulling 1f930c1 on npezza93:add-bell-sound into f3d375a on sourcelair:v3.

@Tyriar
Copy link
Member

Tyriar commented Aug 19, 2017

Woo! Let's merge 😄

@Tyriar Tyriar merged commit 9ab17de into xtermjs:v3 Aug 19, 2017
@Tyriar
Copy link
Member

Tyriar commented Aug 19, 2017

Thanks for the contribution @npezza93 🎆

@Tyriar Tyriar added this to the 3.0.0 milestone Aug 19, 2017
@npezza93 npezza93 deleted the add-bell-sound branch August 19, 2017 20:08
@Tyriar Tyriar mentioned this pull request Aug 19, 2017
9 tasks
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.

6 participants